Remove condition/c_if, duration, and unit from instructions #13506
Conversation
mtreinish
added
on hold
This commit removes the per instruction attributes for condition, duration, and unit. These attributes were deprecated in 1.3.0. Besides the label these attributes were the only mutable state for singleton instructions and removing them simplifies what needs to be tracked for instructions in both Python and rust. The associated methods and classes that were previously dependent on these attributes are also removed as they no longer serve a purpose. The removal of condition in particular removes a lot of logic from Qiskit especially from the transpiler because it was a something that needed to be checked outside of the normal data model for every operation. This PR simplifies the representation of this extra mutable state in Rust by removing the `ExtraInstructionAttributes` struct and replacing it with a `Option<Box<String>>`. The `Option<Box<String>>` is used instead of the simpler `Option<String>` because this this reduces the size of the label field from 24 bytes for `Option<String>` to 8 bytes for `Option<Box<String>>` with an extra layer of pointer indirection and a second heap allocation. This will have runtime overhead when labels are set, but because the vast majority of operations don't set labels the tradeoff for optimizing for the None case makes more sense. Another option would have been to use `Box<str>` here which is the equivalent of `Box<[u8]>` (where `String` is the equivalent to `Vec<u8>`) and from a runtime memory tradeoff would be a better choice for this application if labels were commonly used as labels aren't growable. But that requires 16 bytes instead of 8 and we'd be wasting an additional 8 bytes for each instruction in the circuit which isn't worth the cost.
|
Is there a replacement method for getting circuit timing if |
|
I pushed up a few commits, but right now, I think there's trouble in the scheduling transpiler passes, in that some of the logic around converting |
There was a problem hiding this comment.
13b6814 simplifies the modifications to the scheduling passes, which pulls them back a bit more to what I'd expect. Somebody else ought to review that commit, though.
Since I hadn't last time, I looked through all the tests too. I think there's a couple that were maybe removed by accident, but not certain.
I'm still not wild about the stability around the scheduling passes suddenly needing InstructionDurations (or Target) in 2.0 when it wasn't necessary in 1.x. I get why we have to do that, and I guess I'm saying I'm ok to accept it as a break/exception to the deprecation-policy, but only really because we don't have a choice at this point, other than keeping around state that we really don't want to keep alive any longer.
Other than these last few comments, I'm ok to approve.
| self.assertAlmostEqual(result.values[0], 0, places=1) | ||
| self.assertAlmostEqual(float(result.values[0]), -1.0, places=1) |
There was a problem hiding this comment.
Should this have changed? I feel like it still ought to be averaging to 0. Maybe a bug in Aer?
There was a problem hiding this comment.
It shouldn't have changed as the condition should be identical. It would either be an aer bug or an issue in the primitive implementation (since the BackendEstimatorV2 version didn't require any update). I opted to just roll will it here because this test will also be removed with the BackendEstimator v1 estimator implementation and the v2 tests didn't need any changes.
There was a problem hiding this comment.
Sounds like it must be a bug - I feel like we should either delete this test (and the sampler one) slightly early, or put a big comment on it saying that we're asserting something incorrect. I don't like merging a PR that encodes an incorrect test, even if we're expecting it to be ephemeral.
| self.assertDictAlmostEqual(result.quasi_dists[0], {0: 0.5029296875, 1: 0.4970703125}) | ||
| self.assertDictAlmostEqual(result.quasi_dists[0], {0: 0.021484375, 1: 0.978515625}) |
There was a problem hiding this comment.
This also looks incorrect now - maybe an Aer bug?
There was a problem hiding this comment.
I did the same thing here as for the estimator implementation.
|
My only remaining comment is about the buggy backend primitives V1 tests - I don't mind if we eagerly remove them or put in a comment explaining that they're bugged, but I don't want to merge a broken test, just out of principle. Otherwise: somebody else should confirm that my commit 13b6814 is fine, and then I can approve. |
I'll remove them then, I have the PR removing the backend primitive v1 open already here: #13877 which deletes both test files in their entirety.
It looks fine to me, I think that pass will need more of a refactor pretty soon. The whole mutating the local durations object based on the dag doesn't really work after this PR and the removal of pulse, but for now it should be fine. |
Two primitives tests were returning incorrect results after being rewritten to use IfElseOp instead of c_if. This is likely pointing to a bug in the primitive implementation or an aer bug. But since those classes are deprecated and the tests will be removed as part of the larger backend v1 primitive implementation removal in Qiskit#13877 anyway this commit opts to just delete these tests now.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Summary
This commit removes the per instruction attributes for condition, duration, and unit. These attributes were deprecated in 1.3.0. Besides the label these attributes were the only mutable state for singleton instructions and removing them simplifies what needs to be tracked for instructions in both Python and rust. The associated methods and classes that were previously dependent on these attributes are also removed as they no longer serve a purpose. The removal of condition in particular removes a lot of logic from Qiskit especially from the transpiler because it was a something that needed to be checked outside of the normal data model for every operation.
This PR simplifies the representation of this extra mutable state in Rust by removing the
ExtraInstructionAttributesstruct and replacing it with aOption<Box<String>>. TheOption<Box<String>>is used instead of the simplerOption<String>because this this reduces the size of the label field from 24 bytes forOption<String>to 8 bytes forOption<Box<String>>with an extra layer of pointer indirection and a second heap allocation. This will have runtime overhead when labels are set, but because the vast majority of operations don't set labels the tradeoff for optimizing for the None case makes more sense. Another option would have been to useBox<str>here which is the equivalent ofBox<[u8]>(whereStringis the equivalent toVec<u8>) and from a runtime memory tradeoff would be a better choice for this application if labels were commonly used as labels aren't growable. But that requires 16 bytes instead of 8 and we'd be wasting an additional 8 bytes for each instruction in the circuit which isn't worth the cost.Details and comments
TODO:
c_ifusage in tests (there's >200 instances of it still)