Skip to content

Conversation

@taladjidi
Copy link

Here are some preliminary comments / slight reformatting in the context of the JOSS review:

  • As a general remark, there is a lot of ND array type logic that is a bit hard to follow: branching often occurs to distinguish Vector / Matrix cases, and according to the number of dimension. This induces a lot of redundancy and makes it quite hard to follow what is going on.
  • Some more inheritance could help limit code duplication and shrink the codebase a bit.
  • There were a lot of imports within function. I could not find a reason for it so I removed them (most of them were already at the top of the file anyway).
  • There are a lot of assertions. While this is good because it sanitizes the arguments of functions etc .. it disrupts code readability a lot. I would suggest that these assertions should rather go in the test cases (especially the type assertions which are not present in the tests currently).
  • Some of the typing is a bit inconsistant, a static type checker like Mypy will help you do things more easily.
  • For formatting, it is a bit inconsistent accross the code base, using a precommit hook that runs ruff (or whatever formatter you like) will also help you keep things more tidy and catch a lot of bugs.

Let me know if you have any questions of if I can help in any way :)

Copy link
Member

@SimonSekavcnik SimonSekavcnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We agree with most of your comments and suggestions. We had some comments with the documentation updates that you propose as well as with the inheritance of the CompositeEnvelope.

\hat{\sigma_y} = \begin{bmatrix}
0 & -i \\
i & 0
0 & -Id \\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case i represents imaginary number, not identity matrix.

.. math::
\hat{S} = \begin{bmatrix} 1 & 0 \\ 0 & i \end{bmatrix}
\hat{S} = \begin{bmatrix} 1 & 0 \\ 0 & Id \end{bmatrix}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case i also represents imaginary number.

Constructs T (:math:`\hat T`) operator
.. math::
\hat{T} = \begin{bmatrix} 1 & 0 \\ 0 & e^{i \pi/4} \end{bmatrix}
\hat{T} = \begin{bmatrix} 1 & 0 \\ 0 & e^{Id \pi/4} \end{bmatrix}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, i represents imaginary number.

.. math::
\hat{SX} = \frac{1}{2} \begin{bmatrix} 1+i & 1-i \\ 1-i & 1+i \end{bmatrix}
\hat{SX} = \frac{1}{2} \begin{bmatrix} 1+Id & 1-Id \\ 1-Id & 1+Id \end{bmatrix}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, i represents imaginary number.

\cos\left(\frac{\theta}{2}\right) & -i\sin\left(\frac{\theta}{2}\right) \\
-i\sin\left(\frac{\theta}{2}\right) & \cos\left(\frac{\theta}{2}\right)
\cos\left(\frac{\theta}{2}\right) & -Id\sin\left(\frac{\theta}{2}\right) \\
-Id\sin\left(\frac{\theta}{2}\right) & \cos\left(\frac{\theta}{2}\right)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, i represents imaginary number.

e^{-i\frac{\theta}{2}} & 0 \\
0 & e^{i\frac{\theta}{2}}
e^{-Id\frac{\theta}{2}} & 0 \\
0 & e^{Id\frac{\theta}{2}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, i represents imaginary number.

][0]


# FIXME: Inherit from Envelope to limit code duplication ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CompositeEnvelope has a fundamentally different way of handling states. It does expose similar API, but it handles the states differently. Basically CompositeEnvelope passes the 'commands' to the correct ProductSpace, thus I think that we cannot productively inherit the functionality from the Envelope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants