Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Port synth_cnot_phase_aam to Rust #12937
base: main
Are you sure you want to change the base?
Port synth_cnot_phase_aam to Rust #12937
Changes from 21 commits
8da594c
193058b
2ae9379
d7f41b7
60379b2
671b634
ae91425
64ce56b
fe01589
1ecab6d
a217522
4a36e1e
0b681f9
d2142f6
8982c2c
032b8c2
583ca17
d1394f4
6956e0f
0091c98
92bf2aa
6b2d31e
156da41
b236d62
d28da2a
4c32291
1c7ad1e
68917a0
5973f85
353e5ff
6262d16
41a8906
0cd01ce
75bc902
62aef8c
477e62e
f5a92c9
93375c5
cb2170c
2bdaaae
843c220
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I know the Python code does the same, but this seems highly volatile and we should change this: the input type should be properly differentiated in either floats or strings, but not a mix. You can leave this for now @MozammilQ but maybe we can find a better solution @alexanderivrii and @ShellyGarion 🙂
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.
Indeed, it's quite strange that the API of
angles
can accept strings e.g.s, t, sdg, tdg or z
orfloat
.in the test we see that the angles are only strings, e.g.
s, t, sdg, tdg or z
. It would be good to add some tests of angles which arefloat
to check that the code works well for any angle.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.
@ShellyGarion ,
I had already taken care of this by
angles = [angle if isinstance(angle, str) else f"{angle}" for angle in angles]
incnot_phase_synth.py
. This converts all occurrences of float in the list of angles to String which rust subsequently accepts.Anyways, I have added a test. Take a note of the change in circuit. I have just changed the circuit in the docstring to the actual circuit I am getting here locally, but not have changed the circuit in the code, just to make sure the code works for the circuit generated from the older python implementation of
synth_cnot_phase_aam
, there by proving the circuit generated by rust is merely "equivalent" to the circuit generated by python version and not different.Nevertheless, I am surprised because there is no random steps in the rust or the python implementation of the algorithm, so both the rust and python implementation should produce exact same circuit, but this is not the case.
Maybe, the difference is because when I append the output of
synth_pmh
I just do.rev()
but not the actual.inverse()
in python space as it has been done originally in python implementation.I decided to do only
.rev()
thinking inverse of acx
is itself. So, just reversing the strings ofcx
s in the circuit should be equivalent to doing.inverse()
on QuantumCircuit in python space. Please, guide me if you feel I am lost :)