-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrite OpenQASM 3 exporter symbol table #12776
Conversation
One or more of the following people are relevant to this code:
|
a5b0c6d
to
3220c3f
Compare
Marking as high priority because this blocks #12730. |
Pull Request Test Coverage Report for Build 10040870543Details
💛 - Coveralls |
The failing test is fixed by #12775. I'll push a fix to the lint failure once that's merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great so far. I made a few comments.
I still have a couple of questions on the design. When I finish reading exporter.py tomorrow they may evaporate.
EDIT: The questions dissipated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakelishman This is the last part of my review.
Above, I mentioned a design question. I wondered why a choice was made. I think I see why and mention this in a comment.
This looks like a great improvement. I was not able to go over all of it.
One of the more important comments below is an apparently missing test for a bug fix. (The test still assumes the bug!)
circuit: QuantumCircuit | ||
"""The circuit block that we're currently working on exporting.""" | ||
bit_map: dict[Bit, Bit] | ||
"""Mapping of bit objects in ``circuit`` to the bit objects in the global-scope program |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is readable. I don't think the English-language description needs to be mathematically precise, as long as a reader easily and unconsciously gets a precise understanding.
I'd suggest making the pattern of descriptions uniform, or at least intentionally choosing the pattern based on readability.
In most cases, this is a very minor point. But it also would take only a couple of minutes to edit the 10 or so cases.
It's somewhat more important to make a good choice with more complicated objects, such as "stack of mappings".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you thinking isn't precise about this statement?
qiskit/qasm3/exporter.py
Outdated
"""Track Qiskit objects and the OQ3 identifiers used to refer to them.""" | ||
|
||
def __init__(self): | ||
self.gates: collections.OrderedDict[str, GateDefinition | None] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider keeping everything in the same stack of symbol tables? I suppose you'd have attach a type or other label to each object, in order to do this. But I don't see a lot of types otherwise. To me, keeping the gates separate like you have done looks cleaner and more manageable. Maybe it's the only reasonable way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gates have different rules to variables with regards to definitions, shadowing, etc, so it's a bit clearer (imo) to track them separately - for example, there's no concept of scopes for gate identifiers, since they've got to be global definitions and can't be shadowed ever.
We also need to track different information for Gate
objects compared to other symbols, so it's convenient to put them in a separate table.
TLDR; jakelishman#25 and jakelishman#24 fix a couple of oversights/bugs in this PR. |
This rewrites the symbol handling of the OpenQASM 3 exporter to decouple object identities from the necessary object identifiers. As part of this, we use the same trick of standard-gate reparametrisation to produce gate-definition sources for Qiskit built-ins, which fixes many cases of bad parametrisation of gates like `rzx`. This kind of rewrite was necessary to fix the now-bad assumption within the OQ3 exporter that "gate identity" is always static within a circuit. Since standard gate `Gate` instances are now only generated on demand, there is no guarantee of stability of them. The fix to the definition source for these makes them independent of object identity. User-defined gates can still use the identity, as these are still guaranteed static. This commit fixes almost all of the "bad parametrisation" tests in the test suite. There are several other changes in the test suite necessary: * since the uniqueness of the identifier is now independent of how the lookup of a Qiskit object works, there is no need to include the highly non-deterministic `id` in the generated symbols for user gates. Several tests changed to use the new, simple count-based unique names. * the escaping and uniqueness rules now apply uniformly to all gate definitions, fixing several bad test cases that previously were testing invalid OpenQASM 3. * the escaping rules changed slightly for naming collisions with keywords, making them slightly more consistent with how other renaming rules worked.
If a custom qiskit gate is given a name that is a valid identifer for a hardware qubit in OQ3, then, before this commit, the name would not be escaped when writing the OQ3 gate definition. This commit fixes this by escaping the leading dollar sign as it would be in any other position in the name. That is, the dollar sign is replaced by underscore. Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Much of what we're doing with the "definition source" is actually a form of object canonicalisation for comparison purposes. It's clearer to use this terminology.
74802fd
to
b51386f
Compare
I've addressed all of the above actionable comments. I left the preposition/plurality in "mapping of X(s) to Y(s)" as-is, since I don't think there's any meaningful difference, and we already use both "mapping of" and "mapping from" with plurals and singulars throughout Qiskit. |
The last couple of remaining comments are about potentially improving internal developer documentation in the exporter. That could well be valuable for the future, but let's do it in a follow-up if so, so we can get this merged and unblock #12730 in the interim. |
If you've reviewed the language around mappings and are satisfied, then so am I. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too!
# NOTE: this isn't exactly what we want; note that the parameters in the signature are not | ||
# actually used. It would be fine to change the output of the exporter to make `custom` non | ||
# parametric in this case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(just to make sure I am following) This isn't what we want because we want the parameters in the signature to be used in the definition instead of pi
/pi/4
? Where would the parameter value be stored then?
I can see that at least the issue from #7335 (the value being copied over) isn't there anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, the output of this test should be
OPENQASM 3.0;
include "stdgates.inc";
qubit[2] q;
gate custom(p0, p1) q0, q1 {
rz(p0) q0;
rz(p1 / 2) q1;
}
custom(pi, pi/2) q[0], q[1];
since that's how the custom
gate is originally defined in Qiskit. We can't actually do that, though, because the prior call to assign_parameters
erases the original parametric definition, so we can't find it again. The test as written doesn't use p0
or p1
.
With this PR, this "bad" behaviour is only limited to custom user gates, though, so it's not such a huge deal anymore, as it's rather less likely to come up in practice.
* Rewrite OpenQASM 3 exporter symbol table This rewrites the symbol handling of the OpenQASM 3 exporter to decouple object identities from the necessary object identifiers. As part of this, we use the same trick of standard-gate reparametrisation to produce gate-definition sources for Qiskit built-ins, which fixes many cases of bad parametrisation of gates like `rzx`. This kind of rewrite was necessary to fix the now-bad assumption within the OQ3 exporter that "gate identity" is always static within a circuit. Since standard gate `Gate` instances are now only generated on demand, there is no guarantee of stability of them. The fix to the definition source for these makes them independent of object identity. User-defined gates can still use the identity, as these are still guaranteed static. This commit fixes almost all of the "bad parametrisation" tests in the test suite. There are several other changes in the test suite necessary: * since the uniqueness of the identifier is now independent of how the lookup of a Qiskit object works, there is no need to include the highly non-deterministic `id` in the generated symbols for user gates. Several tests changed to use the new, simple count-based unique names. * the escaping and uniqueness rules now apply uniformly to all gate definitions, fixing several bad test cases that previously were testing invalid OpenQASM 3. * the escaping rules changed slightly for naming collisions with keywords, making them slightly more consistent with how other renaming rules worked. * Add test for bug fix for issue Qiskit#7335 * Rename qiskit gates whose names are OQ3 hardware qubit identifiers If a custom qiskit gate is given a name that is a valid identifer for a hardware qubit in OQ3, then, before this commit, the name would not be escaped when writing the OQ3 gate definition. This commit fixes this by escaping the leading dollar sign as it would be in any other position in the name. That is, the dollar sign is replaced by underscore. Co-authored-by: Jake Lishman <jake.lishman@ibm.com> * Reduce overloading of word "definition" Much of what we're doing with the "definition source" is actually a form of object canonicalisation for comparison purposes. It's clearer to use this terminology. * Remove unnecessary getattr * Fix isinstance/issubclass naming --------- Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
* Rewrite OpenQASM 3 exporter symbol table This rewrites the symbol handling of the OpenQASM 3 exporter to decouple object identities from the necessary object identifiers. As part of this, we use the same trick of standard-gate reparametrisation to produce gate-definition sources for Qiskit built-ins, which fixes many cases of bad parametrisation of gates like `rzx`. This kind of rewrite was necessary to fix the now-bad assumption within the OQ3 exporter that "gate identity" is always static within a circuit. Since standard gate `Gate` instances are now only generated on demand, there is no guarantee of stability of them. The fix to the definition source for these makes them independent of object identity. User-defined gates can still use the identity, as these are still guaranteed static. This commit fixes almost all of the "bad parametrisation" tests in the test suite. There are several other changes in the test suite necessary: * since the uniqueness of the identifier is now independent of how the lookup of a Qiskit object works, there is no need to include the highly non-deterministic `id` in the generated symbols for user gates. Several tests changed to use the new, simple count-based unique names. * the escaping and uniqueness rules now apply uniformly to all gate definitions, fixing several bad test cases that previously were testing invalid OpenQASM 3. * the escaping rules changed slightly for naming collisions with keywords, making them slightly more consistent with how other renaming rules worked. * Add test for bug fix for issue Qiskit#7335 * Rename qiskit gates whose names are OQ3 hardware qubit identifiers If a custom qiskit gate is given a name that is a valid identifer for a hardware qubit in OQ3, then, before this commit, the name would not be escaped when writing the OQ3 gate definition. This commit fixes this by escaping the leading dollar sign as it would be in any other position in the name. That is, the dollar sign is replaced by underscore. Co-authored-by: Jake Lishman <jake.lishman@ibm.com> * Reduce overloading of word "definition" Much of what we're doing with the "definition source" is actually a form of object canonicalisation for comparison purposes. It's clearer to use this terminology. * Remove unnecessary getattr * Fix isinstance/issubclass naming --------- Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
Summary
This rewrites the symbol handling of the OpenQASM 3 exporter to decouple object identities from the necessary object identifiers. As part of this, we use the same trick of standard-gate reparametrisation to produce gate-definition sources for Qiskit built-ins, which fixes many cases of bad parametrisation of gates like
rzx
.This kind of rewrite was necessary to fix the now-bad assumption within the OQ3 exporter that "gate identity" is always static within a circuit. Since standard gate
Gate
instances are now only generated on demand, there is no guarantee of stability of them. The fix to the definition source for these makes them independent of object identity. User-defined gates can still use the identity, as these are still guaranteed static.This commit fixes almost all of the "bad parametrisation" tests in the test suite. There are several other changes in the test suite necessary:
since the uniqueness of the identifier is now independent of how the lookup of a Qiskit object works, there is no need to include the highly non-deterministic
id
in the generated symbols for user gates. Several tests changed to use the new, simple count-based unique names.the escaping and uniqueness rules now apply uniformly to all gate definitions, fixing several bad test cases that previously were testing invalid OpenQASM 3.
the escaping rules changed slightly for naming collisions with keywords, making them slightly more consistent with how other renaming rules worked.
Details and comments
This unblocks #12730 by fixing the broken assumption about object identity for standard gates.
Fix Some internal polishing is needed in the QASM3 exporter #7136 (not to say it's perfect now, but this addresses all the problems tracked by that issue).
Fix QASM 2 and 3 exporters do not handle parameterised gates correctly #7335
Fix QASM 3 exporter makes multiple definitions for same gate #9428
Fix qasm3 returns wrong results with pauli evolution gate #11954
Fix Basis gate set with RXX gate converted with QASM3 and run with Aer simulator results in 'unknown instruction: rxx_139974866104288' #11560
Fix Qiskit’s OpenQASM 3 exporter: not correctly escaping the names of custom gates it’s defining #11761
Supersede Make fewer accesses,
instruction.operation
, in exporter.py #12481This fixes the underlying bug that PR 12481 was attempting to address, though reducing the use of
instruction.operation
as that PR does could still be a benefit to performance.