Skip to content
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

QASM 2 and 3 exporters do not handle parameterised gates correctly #7335

Closed
jakelishman opened this issue Dec 1, 2021 · 3 comments · Fixed by #12776
Closed

QASM 2 and 3 exporters do not handle parameterised gates correctly #7335

jakelishman opened this issue Dec 1, 2021 · 3 comments · Fixed by #12776
Labels
bug Something isn't working mod: qasm2 Relating to OpenQASM 2 import or export mod: qasm3 Related to OpenQASM 3 import or export

Comments

@jakelishman
Copy link
Member

Environment

  • Qiskit Terra version: 0.19.dev @ 2c402dd
  • Python version: any
  • Operating system: any

What is happening?

The QASM3 exporter (and the QASM2 exporter) does not handle arbitrary parameterised gates correctly. It emits a gate definition which includes parameters in the signature, but hardcodes them in the body to the first instance of the gate it found. This is because there's currently no way in Terra to get from a gate with its parameters assigned back to the fully parameterised version.

How can we reproduce the issue?

In [1]: import qiskit.qasm3
   ...: qc = qiskit.QuantumCircuit(2, 2)
   ...: qc.rzx(0.2, 0, 1)
   ...: qc.rzx(0.3, 0, 1)
   ...:
   ...: print(qiskit.qasm3.dumps(qc))
OPENQASM 3;
include "stdgates.inc";
gate rzx(_gate_p_0) _gate_q_0, _gate_q_1 {
  h _gate_q_1;
  cx _gate_q_0, _gate_q_1;
  rz(0.2) _gate_q_1;
  cx _gate_q_0, _gate_q_1;
  h _gate_q_1;
}
bit[2] c;
qubit[2] _all_qubits;
let q = _all_qubits[0:1];
rzx(0.2) q[0], q[1];
rzx(0.3) q[0], q[1];

The two rzx calls have different parameters, but the parameter is hardcoded to 0.2 in the actual definition.

What should happen?

A full fix of this issue will have the rz line in the rzx gate definition use the gate parameter.

Any suggestions?

As a temporary measure before the 0.19 release, we can hack the exporter to always output a new declaration for each instance of every gate that is not in the include files. This is pretty awful, and the output QASM won't correctly represent the structure of the program with re-used gates, but it will at least produce the correct behaviour.

@jakelishman jakelishman added the bug Something isn't working label Dec 1, 2021
@kdk kdk added this to the 0.19 milestone Dec 1, 2021
@kdk kdk added the mod: qasm2 Relating to OpenQASM 2 import or export label Dec 1, 2021
jakelishman added a commit to jakelishman/qiskit-terra that referenced this issue Dec 1, 2021
There were previously parts of the QASM 3 exporter where it attempted to
do its best guess at what should happen, even when this always produced
entirely incorrect results.  These were subroutine definitions with
parameters, opaque gates (`defcal`) and non-included gates with
parameters.

We now choose to fail loudly with a message that we do not currently
support certain features, rather than outputting a meaningless
programme.  For the parameterised gates, we now force the exporter to
output a separate definition for every instance of a gate if it cannot
be sure that it has the most general form to start with.  The code it
outputs looks silly (the gates take parameters, but do not use them in
their definitions, and each gate may be defined many times), but the
semantics of the programme will actually be correct.

See Qiskitgh-7335 for the parameterisation problem.
mergify bot added a commit that referenced this issue Dec 2, 2021
There were previously parts of the QASM 3 exporter where it attempted to
do its best guess at what should happen, even when this always produced
entirely incorrect results.  These were subroutine definitions with
parameters, opaque gates (`defcal`) and non-included gates with
parameters.

We now choose to fail loudly with a message that we do not currently
support certain features, rather than outputting a meaningless
programme.  For the parameterised gates, we now force the exporter to
output a separate definition for every instance of a gate if it cannot
be sure that it has the most general form to start with.  The code it
outputs looks silly (the gates take parameters, but do not use them in
their definitions, and each gate may be defined many times), but the
semantics of the programme will actually be correct.

See gh-7335 for the parameterisation problem.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@mtreinish mtreinish modified the milestones: 0.19, 0.20 Dec 2, 2021
@mtreinish
Copy link
Member

I've changed the milestone here to 0.20.0 as the workaround for 0.19.0 has merged and the proper fix will be for the next release.

@jakelishman jakelishman changed the title QASM 3 exporter does not handle parameterised gates correctly QASM 2 and 3 exporters do not handle parameterised gates correctly Mar 8, 2022
@jakelishman jakelishman modified the milestones: 0.20, 0.21 Mar 28, 2022
@jakelishman
Copy link
Member Author

Changing the milestone to 0.21 because this issue is tied to work in #7624, which also got shifted.

@jakelishman
Copy link
Member Author

This was fixed on the OpenQASM 2 side by #9953 (for Qiskit standard-library gates), and will be fixed in the same manner for OpenQASM 3 by #12776.

There's still a problem with non-standard-library gates whose definitions have bound parameters, but that's generally a problem in Qiskit's data model.

jlapeyre added a commit to jlapeyre/qiskit-core that referenced this issue Jul 19, 2024
jakelishman pushed a commit to jakelishman/qiskit-terra that referenced this issue Jul 22, 2024
github-merge-queue bot pushed a commit that referenced this issue Jul 22, 2024
* 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 #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>
ElePT pushed a commit to mtreinish/qiskit-core that referenced this issue Jul 24, 2024
* 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>
Procatv pushed a commit to Procatv/qiskit-terra-catherines that referenced this issue Aug 1, 2024
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mod: qasm2 Relating to OpenQASM 2 import or export mod: qasm3 Related to OpenQASM 3 import or export
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants