Have BlueprintCircuit.copy_empty_like return an empty QuantumCircuit
#13782
Conversation
Cryoris
added
stable backport potential
|
One or more of the following people are relevant to this code:
|
There was a problem hiding this comment.
thanks, I hate it (lol)
This definitely does seem to be the right way to go - it's a very awkward interaction with a method called copy_empty_like and the BlueprintCircuit, because you could argue that you'd expect to receive an unbuilt object of the same class, but in practice, I think (almost) nobody actually intends to use the BlueprintCircuit for the blueprint-y stuff, it just gets used because of an implementation detail of the circuit library.
Regardless, 2.0 is probably the right time to make that change.
| circuit = EfficientSU2(2) | ||
| circuit.global_phase = -2 | ||
| circuit.metadata = {"my_legacy": "i was a blueprintcircuit once"} | ||
|
|
||
| cpy = circuit.copy_empty_like() | ||
| expected = QuantumCircuit( | ||
| circuit.num_qubits, global_phase=circuit.global_phase, metadata=circuit.metadata | ||
| ) | ||
| self.assertEqual(cpy, expected) | ||
| self.assertNotIsInstance(cpy, BlueprintCircuit) |
There was a problem hiding this comment.
I don't think the metadata is included in the equality check, so I don't think we'll actually be testing that.
I think we need to trigger the build of circuit and do something that would trigger the build of cpy (if it were a BlueprintCircuit) as well like adding a single gate to it - the not isinstance check is ok, but it's a round-about way of testing the behaviour we actually want, which is "the circuit should actually be empty".
There was a problem hiding this comment.
The assertEqual check should trigger the build -- with the old version of the copy_empty_like that test failed 🙂 The final instance check was just to be very clear it is no longer a blueprint circuit 😛 I can add a method call that would trigger the build explicitly, if it were a blueprint circuit
There was a problem hiding this comment.
(I answered the other chain before I read this one.)
Ok, if assertEqual did actually trigger the build, then I guess it is testing it. I guess my now-implied point is that it wasn't obvious to me that assertEqual would necessarily have triggered the build, so it wasn't 100% clear to me that the test was correct, and having that certainty is a nice property of tests.
There was a problem hiding this comment.
I'll add a bit to ensure it would trigger the building and add a comment 🙂
| cpy = circuit.copy_empty_like() | ||
|
|
||
| circuit.num_qubits = 3 # change the original circuit | ||
|
|
||
| expected = QuantumCircuit(2) # we still expect an empty 2-qubit circuit | ||
| self.assertEqual(cpy, expected) |
There was a problem hiding this comment.
It's good to test mutating circuit to ensure no modifications sneak through in shared state, but it's the _build method getting called on cpy which would be the full test, I think?
There was a problem hiding this comment.
There should be no _build on cpy since that's just a plain QuantumCircuit 🙂
There was a problem hiding this comment.
Yeah I know, because you've fixed it, but the point is that this test doesn't necessarily test the regression behaviour, because there's no code in here that super obviously would have caused a build if your PR didn't actually fix the problem. __eq__ might do it accidentally, but you could easily imagine an optimisation in BlueprintCircuit that would cause it not to.
Might be good to do something to cpy which is API guaranteed to cause a build if it were a BlueprintCircuit before testing for emptiness.
jakelishman
removed
the
stable backport potential
|
I've untagged the "stable backport potential" for now, unless there's a pressing reason to really want to backport this, because it does feel like changing one technically valid interpretation of |
Yep, the
Sounds good, it does feel more like an API change than a bugfix tbh -- if we don't backport it, I'd be happy to just label it that. |
Cryoris
added
Changelog: API Change
|
I added explicit triggers to the building process (with comments) and changed to |
Pull Request Test Coverage Report for Build 13133425249Warning: 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.
Details
💛 - Coveralls |
Summary
Fixes #13535.
Details and comments
The
BlueprintCircuit.copy_empty_likemethod currently returns another instance of aBlueprintCircuitwith it's definition wiped. That means that the definition would just be re-built if any circuit method was called, and the circuit wasn't really empty. As discussed in #13535 fixes this by returning aQuantumCircuittype, not aBlueprintCircuitanymore.