Singletonized Measure and Reset instructions
#11170
Conversation
qiskit-bot
added
the
Community PR
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
mtreinish
added
the
Changelog: New Feature
Pull Request Test Coverage Report for Build 6773342731Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
There was a problem hiding this comment.
Thanks very much for this! It looks good, and it's also pretty good news that there was only one place in Qiskit that was using the instructions as non-singletons (and even then, it'll be almost never used because basically nobody uses conditional measurements or resets with OpenQASM 2).
Could you also add a _singleton_lookup_key to Measure and Reset, using stdlib_singleton_key? They have the same instantiation signature as standard-library gates, so we can use that function. You can see an example in qiskit/circuit/library/standard_gates/x.py in the XGate definition, right under the __init__.
Can we also add a couple of brief tests in test/python/circuit/test_singleton.py that Measure() and Reset() both return singletons, and that QuantumCircuit.measure (and reset) both add singleton instances onto circuits?
Other than that, this is good to go thanks.
|
Thank you so much for the review! Sure sir, will add a commit soon enough covering all these changes. |
|
(Don't worry about pressing "update branch", by the way - our merging procedures involve using a merge train that will automatically handle bringing a PR up to date.) |
Matrixmang0
force-pushed
the
singletonize-instructions
branch
from
3d06883 to
f3b678d
Compare
Matrixmang0
requested review from
ikkoham,
t-imamichi,
nonhermitian,
alexanderivrii,
ShellyGarion,
eggerdj,
wshanks,
woodsp-ibm and
ElePT
as code owners
…set() singleton instructions
Matrixmang0
force-pushed
the
singletonize-instructions
branch
from
f3b678d to
3c459d8
Compare
| def test_return_type_singleton_instructions(self): | ||
| measure = Measure() | ||
| self.assertEqual(measure.__class__.__name__, "_SingletonMeasure") | ||
|
|
||
| reset = Reset() | ||
| self.assertEqual(reset.__class__.__name__, "_SingletonReset") |
There was a problem hiding this comment.
This test isn't quite right - technically it'll pass at the moment, but it's reliant on private internal details of the logic. A singleton is an object which is literally the same object on each return - in Python, that means a is b. You can still the style of assertIs tests above for how we test singletons.
|
|
||
| class ESPReset(Reset): | ||
| pass | ||
|
|
||
| reset_base = Reset() | ||
| esp_reset = ESPReset() | ||
| self.assertIs(esp_reset, ESPReset()) | ||
| self.assertIsNot(esp_reset, reset_base) | ||
| self.assertIs(reset_base.base_class, Reset) | ||
| self.assertIs(esp_reset.base_class, ESPReset) |
There was a problem hiding this comment.
We don't need to duplicate this test for Reset - it was enough to just test it for Measure, since at its heart, it was testing inheritance properties, rather than anything specific to Measure.
| def test_singleton_instruction_integration(self): | ||
| qc = QuantumCircuit(1, 1) | ||
| measure = Measure() | ||
| reset = Reset() | ||
| qc.append(measure, [0], [0]) | ||
| qc.append(reset, [0]) | ||
| self.assertEqual(qc.data[0].operation.__class__.__name__, "_SingletonMeasure") | ||
| self.assertEqual(qc.data[1].operation.__class__.__name__, "_SingletonReset") |
There was a problem hiding this comment.
For this test, the intention was to test the QuantumCircuit.measure and QuantumCircuit.reset methods, rather than the direct use of Measure and Reset constructors, which we tested in the test above this one.
|
Thank you very much! |
|
Thank you so much for the approval! Excited to continue learning and contributing. |
Summary
MeasureandResetDetails and comments
This PR partially addresses the issue #10953. It involves singletonizing
MeasureandResetinstructions. However,Barrieris not singletonized after discussing with the maintainer, as the base singleton code needed some additional machinery to handle instructions likeBarrier.Running
toxtest suite resulted in circular import error, which also has been solved with the guidance of the maintainer.Finally added the release notes, elaborating on the features and upgrades brought upon by this pull request.