Add Expr support to OpenQASM 3 exporter
#10408
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 5605101679
💛 - Coveralls |
This adds support for `Expr` nodes in `condition` fields and `SwitchCaseOp.target`. The general `c_if` condition checking is lifted into an `Expr` node as part of the OQ3 export, so the form of them automatically becomes like the new-style outputs, depending on how `expr.lift_legacy_condition` chooses to raise them to `Expr` nodes. As part of the refactor of the `Expression` subtree of the temporary AST, I removed `SubroutineCall`. Its definition was severely out-of-date (a form from before even the initial arXiv draft), where it was statement-like instead of expression-like. There is currently no support in Qiskit for representing these sorts of subroutines properly, and the OQ3 exporter would produce invalid OQ3 if it encountered something that it thought should be a a subroutine call. This commit fixes that bug, correctly raising an exception that non-unitary instructions are not yet supported for export. This commit also removes the special casing from the `CXGate` into the exporter itself; it was more convenient in the new structure of some internal components, and it's a data model we don't want to support anyway.
|
Now rebased over main. |
This is what the QSS stack expects (they don't like bitstring literals in expressions yet), and is more in line with what we used to do anyway.
| # CX gets super duper special treatment because it's the base of Terra's definition | ||
| # tree, but isn't an OQ3 built-in. We use `isinstance` because we haven't fully | ||
| # fixed what the name/class distinction is (there's a test from the original OQ3 | ||
| # exporter that tries a naming collision with 'cx'). |
There was a problem hiding this comment.
The same comment appears above. We probably don't need it twice. The comments are a bit mysterious. I'm not sure if they should be clarified, or maybe I just have to read more code to understand.
There was a problem hiding this comment.
Which bits of it would you like me to clarify? I had it in both places because I thought they were sufficiently far apart in the code that it warranted it, but I'm happy to reduce them if you think it'd be clearer.
There was a problem hiding this comment.
Since putting the comments in both places was intentional, I'm ok with leaving them both in place.
|
This is just slightly on hold because I have a couple of extra tests that I want to write - the exporter will already pass them, but there's a couple of particular forms of output I want to enshrine in it first. The exporter code won't change as I write them, so this could be reviewed already. Actually, I suppose it can even merge without me writing the tests - I can add them later if necessary. |
|
I've added the test case I was worried about now, so this PR is totally ready. |
| TAU = enum.auto() | ||
|
|
||
|
|
||
| class IntegerLiteral(Expression): |
There was a problem hiding this comment.
Renaming these classes with correct names is a great idea. There seem to be a number of these improvements that are unrelated to the PR. One might argue for doing this in a separate PR. But in the interest of development efficiency, I think it's ok to include in this PR.
There was a problem hiding this comment.
In some senses there's not a lot of renaming going on here - most of the classes I removed weren't actually used, and I was putting in new nodes for the actual Expr tree. All previous ParameterExpression values were being handled by the base ast.Expression node, which I effectively renamed to ast.StringifyAndPray to make it clearer what was actually going on, and so I had a more sane base ast.Expression node to work from.
I think IntegerLiteral might be the only one that logically existed in the old form that still exists now but with a different name.
Fwiw, I think this AST module will be completely gone by the end of the year; once we have our custom importer, we can use the AST from that, rather than needed to maintain a parallel AST and formatter in Terra.
Summary
This adds support for
Exprnodes inconditionfields andSwitchCaseOp.target. The generalc_ifcondition checking is lifted into anExprnode as part of the OQ3 export, so the form of them automatically becomes like the new-style outputs, depending on howexpr.lift_legacy_conditionchooses to raise them toExprnodes.As part of the refactor of the
Expressionsubtree of the temporary AST, I removedSubroutineCall. Its definition was severely out-of-date (a form from before even the initial arXiv draft), where it was statement-like instead of expression-like. There is currently no support in Qiskit for representing these sorts of subroutines properly, and the OQ3 exporter would produce invalid OQ3 if it encountered something that it thought should be a a subroutine call. This commit fixes that bug, correctly raising an exception that non-unitary instructions are not yet supported for export.This commit also removes the special casing from the
CXGateinto the exporter itself; it was more convenient in the new structure of some internal components, and it's a data model we don't want to support anyway.Details and comments
Close #10225. Depends on #10249 and #10358. Further changelog in #10331.
I haven't verified the exact output formats from this against what the QSS stack supports yet, but will do early next week and make whatever tweaks are required.