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
Add 2q fractional gates to the
UnitarySynthesis
tranpiler pass #13568base: main
Are you sure you want to change the base?
Add 2q fractional gates to the
UnitarySynthesis
tranpiler pass #13568Changes from 2 commits
89ca81e
077ecb9
18b20f6
967da13
3bb10fd
14c7aac
eeff4cd
043a795
b94070a
0a1644b
48fec9b
009d87e
85e2962
c5d0c97
d7b84e9
ea9a2d0
132a44f
b1cb5a0
9cdf2da
1be749d
1495781
231af7f
bb874c1
c2a9ad4
d39c005
106ae4a
9131e3d
6a82649
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.
Double checking our discussion from earlier in the week: for the target-aware code we discussed adding a third check (apart from
is_controlled
oris_supercontrolled
) to differentiate cases for the 3 available decomposers:TwoQubitDecomposer
,XXDecomposer
, andTwoQubitControlledUDecomposer
. However, for the Python code (just basis gates), it looks like we are simply substituting theXXDecomposer
for theTwoQubitControlledUDecomposer
. Is this the plan?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.
In the case of basis gates, I'm not sure if we can have 3 options or only 2 (this is why this code is only commented out and not removed yet).
If a user provides the basis gates
[rzz, rz, ry]
how can we tell if they want anRZZGate
with an arbitrary angle or anRZZGate
with a fixed angle (and if so - which fixed value)?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.
Another comment: I ran the tests in
qiskit/test/python/transpiler/test_unitary_synthesis.py
(main branch), and couldn't find any test that goes into the following "if" statement (lines 181-188 above), so I wonder if theXXDecomposer
here has ever been 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.
I am not surprised, the coverage of these tests is not good because a lot of the tests are in the synthesis folder and not testing the transpilation pipeline, but calling the synthesis methods directly. Maybe this is a good opportunity to do some test refactoring before merging this PR. During the Rust migration I also noticed there weren't any tests for the
UnitarySynthesis
class that used the QSD method with a target and had to add a couple of them. I think it would make sense to ensure that we have a couple of tests per decomposer, and that there are sufficient tests for the target (HW-aware) path (this I could do in a separate PR because I didn't have time to look more into it during the Rust migration).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 think this is up to us to decide and document the behavior change. I am not sure how many users actually rely or expect the fixed 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.
FYI, I started the test cleanup with this PR: #13582, that is not much but I think can be helpful for future development. Let me know if you have any thoughts on it!
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.
Another question is what to expect when given a parametrized gate in the target? see e.g this test.
Currently, we replace the parametrized gates by gates with a constant angle, without even checking if this angle is actually calibrated in the target:
qiskit/qiskit/transpiler/passes/synthesis/unitary_synthesis.py
Line 801 in 079f69f
Again, I could not find any test that checks the
XXDecomposer
with a given target, namely the tests don't go into here.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.
It seems that
_decomposer_2q_from_target
is not being tested at all, since the tests forUnitarySynthesis
pass for the target use the rust code.Should this function be removed or updated to include
TwoQubitControlledUDecomposer
? (and then some tests should be added).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.
Since the function _decomposer_2q_from_target is part of the
DefaultUnitarySynthesis
plugin we don't update it as part of this PR. We should consider porting this code to rust in a follow-up PR.