-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes gate label import #63
Conversation
@@ -57,7 +57,7 @@ | |||
'gate_expand_1toN', 'gate_expand_2toN', 'gate_expand_3toN', | |||
'qubit_clifford_group', 'expand_operator', '_single_qubit_gates', | |||
'_para_gates', '_ctrl_gates','_swap_like','_toffoli_like', | |||
'_fredkin_like'] | |||
'_fredkin_like','_GATE_NAME_TO_LABEL','_gate_label'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't add these _
constants and functions to __all__
I don't think and we don't need to import _GATE_NAME_TO_LABEL
in circuit.py
at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to import _GATE_NAME_TO_LABEL in circuit.py at all
We do actually. It's used in circuit.py
. I tried running one of the notebooks with _gate_label
and that caused some errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ ag _GATE_NAME_TO_LABEL .
src/qutip_qip/operations/gates.py
171: if (name in _GATE_NAME_TO_LABEL) and (arg_value is not None):
226:_GATE_NAME_TO_LABEL = {
263: if name in _GATE_NAME_TO_LABEL:
264: gate_label = _GATE_NAME_TO_LABEL[name]
I'm not seeing it used anywhere other than in gates.py
? Could you provide a link to where else it is used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was confused between _gate_label
and _GATE_NAME_TO_LABEL
. You are right. Only _gate_label
has to be imported.
Although, for this issue Boxi wanted a separate PR. Feel free to close this PR if that's changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I was asking for a separate PR mainly because the decomposition is a new feature and this is a bug fix. Mixing them might cause some confusion if someone checks the history one day. As #66 is all about bug fixing and cleaning, I can just merge that one and saves you the trouble of updating it for _GATE_NAME_TO_LABEL
. But thank you anyway!
@purva-thakre Thank you for noticing this. I came across it independently and addressed it in a slightly different in #66. If #66 is merged, I'll close this PR. Apologies for not finding this PR before I fixed the bug in mine. |
#66 also adds a test for |
No problem @hodgestar |
Fixes import of gate labels when gates were moved to operations folder. See related comment.
Tested with examples in this notebook.
The notebook also needs a separate PR to change how gates are imported + deprecation warning in
In [62]
. This will be added later.Following code block is for future reference in relation to
In [62]
: