Support standalone expr.Var in QPY
#11651
Conversation
jakelishman
added
Changelog: New Feature
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 8893323077Warning: 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 |
|
This is being deferred to 1.1 along with the rest of the |
This necessitates adding some extra definitions to the circuit header. When standalone variables are used, we do not re-encode the entire variable (which would take a miniumum of ~20 bytes per variable), but instead just emit an index into the order that the variables were defined at the top of the circuit. At present, each control-flow scope will store the `Var` nodes anew, so it's not a perfect only-store-once system. This is consistent with how registers are handled, though, and the QPY format isn't particularly memory optimised as things stand.
|
Now rebased over all necessary fixes. |
There was a problem hiding this comment.
Overall I think this LGTM. I just left a couple comments and questions inline. The only real blocker is there is a missing field in the HEADER struct documentation I think, and I think we should have some negative tests for trying to dump qpy with an older version that uses classical vars. Also maybe having a backwards compat test that uses vars would be good too.
| struct { | ||
| char uuid_bytes[16]; | ||
| char usage; | ||
| uint16_t name_size; |
There was a problem hiding this comment.
One day I'm expecting for a user to complain that qpy fails when they try to serialize a circuit with a 65536 character circuit name or something like that. But so far I guess people are keeping their names reasonable lengths.
There was a problem hiding this comment.
You might enjoy #12117, in that case haha.
* Support standalone `expr.Var` in QPY This necessitates adding some extra definitions to the circuit header. When standalone variables are used, we do not re-encode the entire variable (which would take a miniumum of ~20 bytes per variable), but instead just emit an index into the order that the variables were defined at the top of the circuit. At present, each control-flow scope will store the `Var` nodes anew, so it's not a perfect only-store-once system. This is consistent with how registers are handled, though, and the QPY format isn't particularly memory optimised as things stand. * Add backwards compatibility tests
Summary
This necessitates adding some extra definitions to the circuit header. When standalone variables are used, we do not re-encode the entire variable (which would take a miniumum of ~20 bytes per variable), but instead just emit an index into the order that the variables were defined at the top of the circuit.
At present, each control-flow scope will store the
Varnodes anew, so it's not a perfect only-store-once system. This is consistent with how registers are handled, though, and the QPY format isn't particularly memory optimised as things stand.Details and comments
I need to write a couple more tests, including control-flow scopes, but this is the system in principle. Equality tests are stymied by not having access to
QuantumCircuit.__eq__withVarbecause of the lack of support inDAGCircuit.Close #10930.