Add `AnnotatedOperation.params` and fix some control issues #12752

Cryoris · 2024-07-10T09:47:22Z

Summary

Adds support for AnnotatedOperation.params and, leveraging that, fixes some issues when controlling standard gates that are parameterized. Closes #10311, #10697 and #12135.

Since this PR also changes AnnotatedOperation it's probably too large for a backport?

Details and comments

AnnotatedOperation.params is a proxy to to base_op.params, if it exists. If the base_op has no params, returns an empty list (as discussed with the team offline).
The annotated argument in Gate.control now is None per default, which allows to set it to True for gates that cannot be eagerly synthesized. A test for each gate that failed previously is added.
The S/Sdg gates didn't return CS/CSdg gates for a single control, that's also been fixed.

qiskit-bot · 2024-07-10T09:47:27Z

One or more of the following people are relevant to this code:

@Cryoris
@Qiskit/terra-core
@ajavadia

coveralls · 2024-07-10T10:12:45Z

Pull Request Test Coverage Report for Build 10113248178

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

97 of 97 (100.0%) changed or added relevant lines in 18 files are covered.
84 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.01%) to 89.739%

Files with Coverage Reduction	New Missed Lines	%
qiskit/primitives/containers/bit_array.py	2	95.4%
crates/qasm2/src/lex.rs	3	92.37%
crates/qasm2/src/parse.rs	6	97.61%
qiskit/providers/fake_provider/generic_backend_v2.py	11	95.07%
qiskit/visualization/transition_visualization.py	62	12.5%

Totals
Change from base Build 10099530973:	0.01%
Covered Lines:	66770
Relevant Lines:	74405

💛 - Coveralls

alexanderivrii

This looks really great!

One thing I am confused about is the new ternary semantics of annotated: with this PR it can be either False, True or None. Would it have been possible to achieve the same result without introducing the None value but by changing the default in the newly added control methods to annotated=True?

Previously the semantics of annotated was as follows: False - means do not create an annotated gate, this was required for backward compatibility. True - means introduce an annotated gate unless there is a friendlier gate available, e.g. x.control(1, annotated=True) would produce a CX-gate, not an annotated operation.

alexanderivrii · 2024-07-10T09:56:50Z

releasenotes/notes/annotated-params-116288d5628f7ee8.yaml

+    they contain unbound parameters. Previously, this raised an error, but
+    now we create an :class:`.AnnotatedOperation` as placeholder. This


Maybe say something like: "Previously, this raised an error when such gates were added to the circuit, but ..."

qiskit/circuit/annotated_operation.py

qiskit/circuit/gate.py

qiskit/circuit/library/standard_gates/multi_control_rotation_gates.py

qiskit/circuit/library/standard_gates/rxx.py

alexanderivrii · 2024-07-10T10:31:14Z

Could you please also a test for reading/setting params for an annotated operation of an annotated operation of a gate?

Cryoris · 2024-07-10T14:24:23Z

Would it have been possible to achieve the same result without introducing the None value but by changing the default in the newly added control methods to annotated=True?

That would've been my preferred solution as well, but I thought the default argument cannot change in between classes derived from the same object. If that's allowed, then it would be the nicer solution 🙂

Edit: but the default value also changes with respect to the number of controls and whether the gate parameters are unbound... so we cannot really set it to True or False in the signature 🤔

alexanderivrii · 2024-07-11T12:40:23Z

Suppose we keep the default annotated=False so as not change any previous behavior, but we modify the semantics of gate.control(n, annotated) to the following:

if annotated is False, then we do not create an annotated operation, unless it depends on parameters
if annotated is True, then we choose the version that is more convenient [as before]

In this way we do not change anything that used to work before, and obtain annotated operations for controlled parametric gates. Personally I prefer this slightly over the ternary semantics, but I am happy to change my opinion.

Cryoris · 2024-07-11T13:15:55Z

The advantage of what you're describing is that we don't have to change the signature, which would be nice as it reduces the amount of required change. But this has two downsides:

We're implicitly changing default values (e.g. changing from False to True in some cases)
We're overriding user inputs (e.g. if a user explicitly sets False but we override it to True)

In the None scenario, I think it is easier to understand what default values are chosen, plus users can still force us to use whatever value they want.

if annotated is True, then we choose the version that is more convenient [as before]

I'm not sure I fully understand: if I do RYGate(theta).control(annotated=True) it still uses an AnnotatedOperation even though a CRYGate exists, which we could use.

alexanderivrii · 2024-07-21T13:07:01Z

Thanks @Cryoris. I am not against merging this PR in the current form, and as it fixes an important set of problems we should definitely include it in 1.2. However, let me try to describe again the two options which I think we have.

First option (as I am proposing). We only have two values annotated=False and annotated=True. I am thinking of these as user-specified hints that the user passes to a gate's control/inverse/power methods, and Qiskit is allowed to override these values. For instance, it can override False to True for controlled parametric gates. And it can override True to False if Qiskit has a native representation of this gate that it can use. Personally, I think that RYGate(theta).control(annotated=True) (with 1 control) should be the CRYGate: since we have it in Qiskit, we should probably use it (in particular, maybe we have a great default definition for this gate). In this case, the default value is always False.

Second option (as you are proposing). We have three values annotated=False, annotated=True, and annotated=None with the default value being None. I am interpreting these as follows. The value None means let Qiskit choose the best possible representation. The value of False should then represent "do not create an AnnotatedOperation and fail horribly when writing RYGate(theta).control(annotated=False)". The value of True means "always introduce an annotated operation, with in particular XGate().control(annotated=True) producing an AnnotatedOperation instead of CXGate()."

After writing this down so explicitly, I actually start liking the second choice much better :). Especially if the default value is None in which case Qiskit decides how the gate should be implemented. And if a user wants to shoot oneself in the foot by providing non-default values, let it be. However, it also gives the user (and transpiler passes) much better control on what the gate really is.

So, provided that we fully agree to go with the second option, the only thing I would like to ask you is to make the semantics of annotated clear both in the documentation and in the release notes. And also please check all the other places where annotated is used to make sure it adheres with this semantics.

- use attribute error - more clearly state the new None arg in reno and Gate class

Cryoris · 2024-07-22T16:30:29Z

Option 2 it is then 🙂 I added more detailed explanations about the None argument and resolved the other comments 👍🏻

ElePT

I think the PR looks very good, just left a few minor wording suggestions.

qiskit/circuit/annotated_operation.py

ElePT · 2024-07-25T13:20:42Z

qiskit/circuit/annotated_operation.py

+                f"Cannot set attribute ``params`` on the base operation {self.base_op}."
+            )
+
+    def validate_parameter(self, parameter: ParameterValueType) -> ParameterValueType:


Small question, would it be more coherent if we called it validate_param? I am a bit on the fence, I tend to prefer full words but I find it strange that the other methods say params and this one says parameter

This is coming from Instruction.validate_parameter, to have AnnotatedOperation work as drop-in-replacement 🙂

qiskit/circuit/library/standard_gates/rx.py

qiskit/circuit/library/standard_gates/rxx.py

releasenotes/notes/annotated-params-116288d5628f7ee8.yaml

ElePT

LGTM! I just added two comma-related minor suggestions. After they are applied I think the PR is ready to be merged.

releasenotes/notes/annotated-params-116288d5628f7ee8.yaml

qiskit/circuit/gate.py

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

* AnnotatedOp.params support and Gate.control fix * add reno * lint * update reno * review comments - use attribute error - more clearly state the new None arg in reno and Gate class * review from Elena * Fix ``AttributeError`` test * lint * Apply suggestions from code review Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> --------- Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> (cherry picked from commit 1512535)

…12828) * AnnotatedOp.params support and Gate.control fix * add reno * lint * update reno * review comments - use attribute error - more clearly state the new None arg in reno and Gate class * review from Elena * Fix ``AttributeError`` test * lint * Apply suggestions from code review Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> --------- Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> (cherry picked from commit 1512535) Co-authored-by: Julien Gacon <gaconju@gmail.com>

…2752) * AnnotatedOp.params support and Gate.control fix * add reno * lint * update reno * review comments - use attribute error - more clearly state the new None arg in reno and Gate class * review from Elena * Fix ``AttributeError`` test * lint * Apply suggestions from code review Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> --------- Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

Cryoris added 4 commits July 10, 2024 11:32

AnnotatedOp.params support and Gate.control fix

7966790

add reno

08c6fbc

lint

5ba6861

update reno

3d118b6

Cryoris added Changelog: New Feature Include in the "Added" section of the changelog Changelog: Bugfix Include in the "Fixed" section of the changelog labels Jul 10, 2024

Cryoris added this to the 1.2.0 milestone Jul 10, 2024

Cryoris requested a review from a team as a code owner July 10, 2024 09:47

alexanderivrii reviewed Jul 10, 2024

View reviewed changes

review comments

148b5c7

- use attribute error - more clearly state the new None arg in reno and Gate class

Cryoris requested a review from ElePT July 22, 2024 16:30

ElePT self-assigned this Jul 23, 2024

ShellyGarion mentioned this pull request Jul 24, 2024

Unable to Create Multi-Controlled Gates with More Than 3 Controls in Qiskit #12801

Closed

ElePT reviewed Jul 25, 2024

View reviewed changes

Cryoris added 4 commits July 25, 2024 17:22

review from Elena

7a03c4f

Fix AttributeError test

8a16f11

Merge branch 'main' into add-annotated-params

756810c

lint

0b5b5b9

ElePT previously approved these changes Jul 26, 2024

View reviewed changes

releasenotes/notes/annotated-params-116288d5628f7ee8.yaml Outdated Show resolved Hide resolved

qiskit/circuit/gate.py Outdated Show resolved Hide resolved

Cryoris dismissed ElePT’s stale review via 891ade7 July 26, 2024 14:51

Apply suggestions from code review

891ade7

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

ElePT approved these changes Jul 26, 2024

View reviewed changes

ElePT enabled auto-merge July 26, 2024 14:54

ElePT added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Jul 26, 2024

ElePT added this pull request to the merge queue Jul 26, 2024

Merged via the queue into Qiskit:main with commit 1512535 Jul 26, 2024
15 checks passed

mergify bot mentioned this pull request Jul 26, 2024

Add AnnotatedOperation.params and fix some control issues (backport #12752) #12828

Merged

This was referenced Jul 29, 2024

Controlled UGate with a parameterized angle fails with 4 or more controls #12135

Closed

parameters in RXXGate #10697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `AnnotatedOperation.params` and fix some control issues #12752

Add `AnnotatedOperation.params` and fix some control issues #12752

Cryoris commented Jul 10, 2024

qiskit-bot commented Jul 10, 2024

coveralls commented Jul 10, 2024 •

edited

Loading

alexanderivrii left a comment

alexanderivrii Jul 10, 2024

alexanderivrii commented Jul 10, 2024

Cryoris commented Jul 10, 2024 •

edited

Loading

alexanderivrii commented Jul 11, 2024 •

edited

Loading

Cryoris commented Jul 11, 2024

alexanderivrii commented Jul 21, 2024

Cryoris commented Jul 22, 2024

ElePT left a comment

ElePT Jul 25, 2024

Cryoris Jul 25, 2024

ElePT left a comment

		they contain unbound parameters. Previously, this raised an error, but
		now we create an :class:`.AnnotatedOperation` as placeholder. This

Add AnnotatedOperation.params and fix some control issues #12752

Add AnnotatedOperation.params and fix some control issues #12752

Conversation

Cryoris commented Jul 10, 2024

Summary

Details and comments

qiskit-bot commented Jul 10, 2024

coveralls commented Jul 10, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10113248178

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

alexanderivrii left a comment

Choose a reason for hiding this comment

alexanderivrii Jul 10, 2024

Choose a reason for hiding this comment

alexanderivrii commented Jul 10, 2024

Cryoris commented Jul 10, 2024 • edited Loading

alexanderivrii commented Jul 11, 2024 • edited Loading

Cryoris commented Jul 11, 2024

alexanderivrii commented Jul 21, 2024

Cryoris commented Jul 22, 2024

ElePT left a comment

Choose a reason for hiding this comment

ElePT Jul 25, 2024

Choose a reason for hiding this comment

Cryoris Jul 25, 2024

Choose a reason for hiding this comment

ElePT left a comment

Choose a reason for hiding this comment

Add `AnnotatedOperation.params` and fix some control issues #12752

Add `AnnotatedOperation.params` and fix some control issues #12752

coveralls commented Jul 10, 2024 •

edited

Loading

Cryoris commented Jul 10, 2024 •

edited

Loading

alexanderivrii commented Jul 11, 2024 •

edited

Loading