Singleton parameterless controlled gates #10898
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds a new class SingletonControlledGate which is very similar to the SingletonGate class, but instead can be used in place of the ControlledGate class as a parent class. This enables a controlled gate class to work as a singleton if there is no custom control state specified. This new class has a large amount of duplication with the SingletonGate class, but because of how the inheritance tree is built up here there is no avoiding this as we need to specialize for adding singleton support and SingletonGate is at the same level as ControlledGate. With the introduction of this new class all the standard library ControlledGate instances are updated so they inherit from SingletonControlledGate instead of ControlledGate. This should significantly reduce the memory footprint of repeated instances of these gates in circuits.
mtreinish
added
Changelog: New Feature
|
One or more of the the following people are requested to review this:
|
|
Ok, this should be ready for review now. I fixed the last failing test cases (all of which were caused by extra state needed for controlled gate reconstruction getting lost through |
|
I ran a subset of ASV just as a sanity check locally and it didn't show any statistically significant change: |
kevinhartman
reviewed
Oct 4, 2023
Co-authored-by: Kevin Hartman <[email protected]>
This was referenced Oct 9, 2023
There was a problem hiding this comment.
Overall LGTM!
My only suggestion would be that the release notes seem focused more heavily on users who are extending Qiskit with additional instructions and gates rather than on the user impact for building circuits. Specifically, I think we might want to communicate more concisely that gates in our circuit library not customized during construction no longer support mutation out of the box.
|
|
||
| def __init__( | ||
| self, | ||
| num_ctrl_qubits: int, | ||
| dirty_ancillas: bool = False, | ||
| label: Optional[str] = None, | ||
| ctrl_state: Optional[Union[str, int]] = None, | ||
| _base_label=None, |
There was a problem hiding this comment.
Does this need to have unit and duration as well?
There was a problem hiding this comment.
This one gets tricky based on the how MCXGate works. For some gates it calls __init__ from __new__ of MCXGate() and tracking the args gets confusing because of the layer of indirection. I'll double check these really quickly to see if they're missing something.
There was a problem hiding this comment.
Yeah, I just double checked for singleton gates in this PR I think we're ok. If the number of controls results in a discrete gate class like CXGate or CCXGate we already have the handling on __new__ which internally calls __init__ for the gate object in MCXGate.__new__ (which is the parent of this). If it's not then it's not a singleton because the gates are parameterized over the number of controls. There is a second PR #11013 which will cover this more broadly, so I'd say lets just save this for that PR.
Comment on lines
73
to
75
| :class:`~.ControlledGate` and its direct subclasses, but mutation of these attributes | ||
| on :class:`~.SingletonControlledGate` and its subclasses is disallowed, and will raise | ||
| and exception. These attributes can be customized but only at creation time (i.e. via |
There was a problem hiding this comment.
This isn't quite right anymore since now SingletonControlledGate is a direct subclass of ControlledGate :3
There was a problem hiding this comment.
I updated this in: 3a108ab let me know if there is more to update there.
There was an existing section on the user upgrade impact of singleton gates from #10314 that I expanded in this PR. I added a bit more detail to it in 3a108ab. But I think we can save deeper tweaks for the final release notes PR, especially since we'll likely be putting singletons in the prelude. |
kevinhartman
approved these changes
Oct 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This commit adds a new class SingletonControlledGate which is very similar to the SingletonGate class, but instead can be used in place of the ControlledGate class as a parent class. This enables a controlled gate class to work as a singleton if there is no custom control state specified. This new class has a large amount of duplication with the SingletonGate class, but because of how the inheritance tree is built up here there is no avoiding this as we need to specialize for adding singleton support and SingletonGate is at the same level as ControlledGate.
With the introduction of this new class all the standard library ControlledGate instances are updated so they inherit from SingletonControlledGate instead of ControlledGate. This should significantly reduce the memory footprint of repeated instances of these gates in circuits.
Details and comments
TODO:
SingletonControlledGatetest cases