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

Introduce Qiskit native ParameterExpression serialization #13252

Closed
mtreinish opened this issue Oct 1, 2024 · 0 comments · Fixed by #13356
Closed

Introduce Qiskit native ParameterExpression serialization #13252

mtreinish opened this issue Oct 1, 2024 · 0 comments · Fixed by #13356
Assignees
Labels
mod: qpy Related to QPY serialization priority: high type: feature request New feature or request
Milestone

Comments

@mtreinish
Copy link
Member

What should we add?

Right now QPY relies on symengine's native serialization format for serializing the internal symbolic expression used by the parameter expression. At the time this made a lot of sense because we could relying on symengine to manage how best to represent it's internal data model and it was also C++ relying on the cereal library so it was very fast. But as was recently discovered by the symengine 0.13 release the payload format is version specific, and symengine will error if you try to load a expression generated with a different symengine version. For the immediate term we were able to "workaround" this issue here: #13251 but this isn't a general solution as symengine could change the format between versions, that's why they have the check.

Moving forward for qiskit 1.3.0 we must come up with a qiskit native serialization for ParameterExpression for QPY instead of relying on symengine or sympy to do it for us. This should be introduced in QPY version 13, this version mismatch is still a possibility for users of QPY version >=10 and < 13 when loading a historically generated payload, but in those cases the only option is to install a compatible version of symengine if the payload format is relying on it.

@mtreinish mtreinish added type: feature request New feature or request priority: high mod: qpy Related to QPY serialization labels Oct 1, 2024
@mtreinish mtreinish added this to the 1.3.0 milestone Oct 1, 2024
mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Oct 1, 2024
Out of an abundance of caution this commit places a cap on the allowed
version of symengine users can install to be compatible with Qiskit.
Due to the symengine version dependence discovered in QPY around
serializing ParameterExpressions, we'll likely have a similar issue
when symengine 0.14.0 releases. Pre-emptively capping this means we
aren't going to be in this situation until we can confirm compatibility
with QPY serialization. The real solution for this will come in Qiskit#13252,
although as this behavior is embedded in QPY formats 10, 11, and 12 at
this point we'll have to handle this edge case moving forward regardless
of whether we introduce a better solution in 1.3.0 or not. Although
realistically in that case we will likely need to just document this as
a limitation when exporting QPY payloads with Qiskit 0.45.0 through
1.2.3 (and with the ``version`` flag set to >= 10 and < 13) and have
explicit error checking around the symengine version (which this PR
adds) when in that code path.
github-merge-queue bot pushed a commit that referenced this issue Oct 2, 2024
* Workaround symengine serialization payload incompatibility

In QPY we rely on symengine's internal serialization to represent the
internal symbolic expression stored inside a ParameterExpression object.
However, this format is nominally symengine version specific and will
raise an error if there is a mismatch between the version used to
generate the payload and what is trying to read it. This became an issue
in the recent symengine 0.13 release which started to raise an error
when people installed it and tried to load QPY payloads across the
versions. This makes the symengine serialization unsuitable for use in
QPY because it's supposed to be independent of these kind of concerns,
especially when QPY is used in a server-client model where you don't
necessarily control the installed environment of symengine.

To correctly address this issue we'll need a new version of the QPY
format that owns the serialization format of ParameterExpressions directly
instead of relying on symengine which doesn't offer a compatibility
guarantee on the format. However this won't be quick solution and users
are encountering issues since the release of 0.13. This commit
introduces a workaround for this specific instance of the mismatch. It
turns out the payload format between 0.11 and 0.13 is completely
unchanged except for the version number. So before passing the parameter
expression payload to symengine for deserialization this commit checks
the versions numbers are the same, if they're not it checks that we're
dealing with 0.11 or 0.13, and if so it changes the version number in
the payloads header appropriately. If the version number is outside
those bounds it raises an exception because while this hack is known to
be safe for translating between symengine 0.11 and 0.13, it's not
possible to know for a future version whether the payload format changed
or not.

Longer term we will need a proper fix in qpy version 13 that introduces
a qiskit native serialization format for parameter expression instead of
relying on symengine or sympy to do it for us.

* Handle schedules too

This commit updates the schedule serialization path too, as it was also
directly loading symengine expressions. The code handling the workaround
is extracted to a standalone function which is used in both spots now
instead of calling symengine directly.

* Remove unused imports

* Gracefully handle failure to parse historical symengine files

During the discovery on the fix in this PR we discovered that setting
the ``use_symengine`` flag from Qiskit 0.45.x and 0.46.x would result in
newer versions of Qiskit being unable to parse the QPY file for the same
issue as being addressed here. This commit expands the logic to account
for this and raise a useful warning. It also updates the release notes
and documentation to document these limitations.

* Cap upper version of symengine in requirements list

Out of an abundance of caution this commit places a cap on the allowed
version of symengine users can install to be compatible with Qiskit.
Due to the symengine version dependence discovered in QPY around
serializing ParameterExpressions, we'll likely have a similar issue
when symengine 0.14.0 releases. Pre-emptively capping this means we
aren't going to be in this situation until we can confirm compatibility
with QPY serialization. The real solution for this will come in #13252,
although as this behavior is embedded in QPY formats 10, 11, and 12 at
this point we'll have to handle this edge case moving forward regardless
of whether we introduce a better solution in 1.3.0 or not. Although
realistically in that case we will likely need to just document this as
a limitation when exporting QPY payloads with Qiskit 0.45.0 through
1.2.3 (and with the ``version`` flag set to >= 10 and < 13) and have
explicit error checking around the symengine version (which this PR
adds) when in that code path.

* Fix release note upgrade section label

* Rewrite support documentation

* Fix mistakes in release note

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: Jake Lishman <jake@binhbar.com>
mergify bot pushed a commit that referenced this issue Oct 2, 2024
* Workaround symengine serialization payload incompatibility

In QPY we rely on symengine's internal serialization to represent the
internal symbolic expression stored inside a ParameterExpression object.
However, this format is nominally symengine version specific and will
raise an error if there is a mismatch between the version used to
generate the payload and what is trying to read it. This became an issue
in the recent symengine 0.13 release which started to raise an error
when people installed it and tried to load QPY payloads across the
versions. This makes the symengine serialization unsuitable for use in
QPY because it's supposed to be independent of these kind of concerns,
especially when QPY is used in a server-client model where you don't
necessarily control the installed environment of symengine.

To correctly address this issue we'll need a new version of the QPY
format that owns the serialization format of ParameterExpressions directly
instead of relying on symengine which doesn't offer a compatibility
guarantee on the format. However this won't be quick solution and users
are encountering issues since the release of 0.13. This commit
introduces a workaround for this specific instance of the mismatch. It
turns out the payload format between 0.11 and 0.13 is completely
unchanged except for the version number. So before passing the parameter
expression payload to symengine for deserialization this commit checks
the versions numbers are the same, if they're not it checks that we're
dealing with 0.11 or 0.13, and if so it changes the version number in
the payloads header appropriately. If the version number is outside
those bounds it raises an exception because while this hack is known to
be safe for translating between symengine 0.11 and 0.13, it's not
possible to know for a future version whether the payload format changed
or not.

Longer term we will need a proper fix in qpy version 13 that introduces
a qiskit native serialization format for parameter expression instead of
relying on symengine or sympy to do it for us.

* Handle schedules too

This commit updates the schedule serialization path too, as it was also
directly loading symengine expressions. The code handling the workaround
is extracted to a standalone function which is used in both spots now
instead of calling symengine directly.

* Remove unused imports

* Gracefully handle failure to parse historical symengine files

During the discovery on the fix in this PR we discovered that setting
the ``use_symengine`` flag from Qiskit 0.45.x and 0.46.x would result in
newer versions of Qiskit being unable to parse the QPY file for the same
issue as being addressed here. This commit expands the logic to account
for this and raise a useful warning. It also updates the release notes
and documentation to document these limitations.

* Cap upper version of symengine in requirements list

Out of an abundance of caution this commit places a cap on the allowed
version of symengine users can install to be compatible with Qiskit.
Due to the symengine version dependence discovered in QPY around
serializing ParameterExpressions, we'll likely have a similar issue
when symengine 0.14.0 releases. Pre-emptively capping this means we
aren't going to be in this situation until we can confirm compatibility
with QPY serialization. The real solution for this will come in #13252,
although as this behavior is embedded in QPY formats 10, 11, and 12 at
this point we'll have to handle this edge case moving forward regardless
of whether we introduce a better solution in 1.3.0 or not. Although
realistically in that case we will likely need to just document this as
a limitation when exporting QPY payloads with Qiskit 0.45.0 through
1.2.3 (and with the ``version`` flag set to >= 10 and < 13) and have
explicit error checking around the symengine version (which this PR
adds) when in that code path.

* Fix release note upgrade section label

* Rewrite support documentation

* Fix mistakes in release note

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: Jake Lishman <jake@binhbar.com>
(cherry picked from commit fee9f77)
github-merge-queue bot pushed a commit that referenced this issue Oct 2, 2024
…13251) (#13255)

* Workaround symengine serialization payload incompatibility (#13251)

* Workaround symengine serialization payload incompatibility

In QPY we rely on symengine's internal serialization to represent the
internal symbolic expression stored inside a ParameterExpression object.
However, this format is nominally symengine version specific and will
raise an error if there is a mismatch between the version used to
generate the payload and what is trying to read it. This became an issue
in the recent symengine 0.13 release which started to raise an error
when people installed it and tried to load QPY payloads across the
versions. This makes the symengine serialization unsuitable for use in
QPY because it's supposed to be independent of these kind of concerns,
especially when QPY is used in a server-client model where you don't
necessarily control the installed environment of symengine.

To correctly address this issue we'll need a new version of the QPY
format that owns the serialization format of ParameterExpressions directly
instead of relying on symengine which doesn't offer a compatibility
guarantee on the format. However this won't be quick solution and users
are encountering issues since the release of 0.13. This commit
introduces a workaround for this specific instance of the mismatch. It
turns out the payload format between 0.11 and 0.13 is completely
unchanged except for the version number. So before passing the parameter
expression payload to symengine for deserialization this commit checks
the versions numbers are the same, if they're not it checks that we're
dealing with 0.11 or 0.13, and if so it changes the version number in
the payloads header appropriately. If the version number is outside
those bounds it raises an exception because while this hack is known to
be safe for translating between symengine 0.11 and 0.13, it's not
possible to know for a future version whether the payload format changed
or not.

Longer term we will need a proper fix in qpy version 13 that introduces
a qiskit native serialization format for parameter expression instead of
relying on symengine or sympy to do it for us.

* Handle schedules too

This commit updates the schedule serialization path too, as it was also
directly loading symengine expressions. The code handling the workaround
is extracted to a standalone function which is used in both spots now
instead of calling symengine directly.

* Remove unused imports

* Gracefully handle failure to parse historical symengine files

During the discovery on the fix in this PR we discovered that setting
the ``use_symengine`` flag from Qiskit 0.45.x and 0.46.x would result in
newer versions of Qiskit being unable to parse the QPY file for the same
issue as being addressed here. This commit expands the logic to account
for this and raise a useful warning. It also updates the release notes
and documentation to document these limitations.

* Cap upper version of symengine in requirements list

Out of an abundance of caution this commit places a cap on the allowed
version of symengine users can install to be compatible with Qiskit.
Due to the symengine version dependence discovered in QPY around
serializing ParameterExpressions, we'll likely have a similar issue
when symengine 0.14.0 releases. Pre-emptively capping this means we
aren't going to be in this situation until we can confirm compatibility
with QPY serialization. The real solution for this will come in #13252,
although as this behavior is embedded in QPY formats 10, 11, and 12 at
this point we'll have to handle this edge case moving forward regardless
of whether we introduce a better solution in 1.3.0 or not. Although
realistically in that case we will likely need to just document this as
a limitation when exporting QPY payloads with Qiskit 0.45.0 through
1.2.3 (and with the ``version`` flag set to >= 10 and < 13) and have
explicit error checking around the symengine version (which this PR
adds) when in that code path.

* Fix release note upgrade section label

* Rewrite support documentation

* Fix mistakes in release note

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: Jake Lishman <jake@binhbar.com>
(cherry picked from commit fee9f77)

* Fix symengine typo

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
ElePT pushed a commit to ElePT/qiskit that referenced this issue Oct 3, 2024
)

* Workaround symengine serialization payload incompatibility

In QPY we rely on symengine's internal serialization to represent the
internal symbolic expression stored inside a ParameterExpression object.
However, this format is nominally symengine version specific and will
raise an error if there is a mismatch between the version used to
generate the payload and what is trying to read it. This became an issue
in the recent symengine 0.13 release which started to raise an error
when people installed it and tried to load QPY payloads across the
versions. This makes the symengine serialization unsuitable for use in
QPY because it's supposed to be independent of these kind of concerns,
especially when QPY is used in a server-client model where you don't
necessarily control the installed environment of symengine.

To correctly address this issue we'll need a new version of the QPY
format that owns the serialization format of ParameterExpressions directly
instead of relying on symengine which doesn't offer a compatibility
guarantee on the format. However this won't be quick solution and users
are encountering issues since the release of 0.13. This commit
introduces a workaround for this specific instance of the mismatch. It
turns out the payload format between 0.11 and 0.13 is completely
unchanged except for the version number. So before passing the parameter
expression payload to symengine for deserialization this commit checks
the versions numbers are the same, if they're not it checks that we're
dealing with 0.11 or 0.13, and if so it changes the version number in
the payloads header appropriately. If the version number is outside
those bounds it raises an exception because while this hack is known to
be safe for translating between symengine 0.11 and 0.13, it's not
possible to know for a future version whether the payload format changed
or not.

Longer term we will need a proper fix in qpy version 13 that introduces
a qiskit native serialization format for parameter expression instead of
relying on symengine or sympy to do it for us.

* Handle schedules too

This commit updates the schedule serialization path too, as it was also
directly loading symengine expressions. The code handling the workaround
is extracted to a standalone function which is used in both spots now
instead of calling symengine directly.

* Remove unused imports

* Gracefully handle failure to parse historical symengine files

During the discovery on the fix in this PR we discovered that setting
the ``use_symengine`` flag from Qiskit 0.45.x and 0.46.x would result in
newer versions of Qiskit being unable to parse the QPY file for the same
issue as being addressed here. This commit expands the logic to account
for this and raise a useful warning. It also updates the release notes
and documentation to document these limitations.

* Cap upper version of symengine in requirements list

Out of an abundance of caution this commit places a cap on the allowed
version of symengine users can install to be compatible with Qiskit.
Due to the symengine version dependence discovered in QPY around
serializing ParameterExpressions, we'll likely have a similar issue
when symengine 0.14.0 releases. Pre-emptively capping this means we
aren't going to be in this situation until we can confirm compatibility
with QPY serialization. The real solution for this will come in Qiskit#13252,
although as this behavior is embedded in QPY formats 10, 11, and 12 at
this point we'll have to handle this edge case moving forward regardless
of whether we introduce a better solution in 1.3.0 or not. Although
realistically in that case we will likely need to just document this as
a limitation when exporting QPY payloads with Qiskit 0.45.0 through
1.2.3 (and with the ``version`` flag set to >= 10 and < 13) and have
explicit error checking around the symengine version (which this PR
adds) when in that code path.

* Fix release note upgrade section label

* Rewrite support documentation

* Fix mistakes in release note

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: Jake Lishman <jake@binhbar.com>
ElePT pushed a commit to ElePT/qiskit that referenced this issue Oct 7, 2024
)

* Workaround symengine serialization payload incompatibility

In QPY we rely on symengine's internal serialization to represent the
internal symbolic expression stored inside a ParameterExpression object.
However, this format is nominally symengine version specific and will
raise an error if there is a mismatch between the version used to
generate the payload and what is trying to read it. This became an issue
in the recent symengine 0.13 release which started to raise an error
when people installed it and tried to load QPY payloads across the
versions. This makes the symengine serialization unsuitable for use in
QPY because it's supposed to be independent of these kind of concerns,
especially when QPY is used in a server-client model where you don't
necessarily control the installed environment of symengine.

To correctly address this issue we'll need a new version of the QPY
format that owns the serialization format of ParameterExpressions directly
instead of relying on symengine which doesn't offer a compatibility
guarantee on the format. However this won't be quick solution and users
are encountering issues since the release of 0.13. This commit
introduces a workaround for this specific instance of the mismatch. It
turns out the payload format between 0.11 and 0.13 is completely
unchanged except for the version number. So before passing the parameter
expression payload to symengine for deserialization this commit checks
the versions numbers are the same, if they're not it checks that we're
dealing with 0.11 or 0.13, and if so it changes the version number in
the payloads header appropriately. If the version number is outside
those bounds it raises an exception because while this hack is known to
be safe for translating between symengine 0.11 and 0.13, it's not
possible to know for a future version whether the payload format changed
or not.

Longer term we will need a proper fix in qpy version 13 that introduces
a qiskit native serialization format for parameter expression instead of
relying on symengine or sympy to do it for us.

* Handle schedules too

This commit updates the schedule serialization path too, as it was also
directly loading symengine expressions. The code handling the workaround
is extracted to a standalone function which is used in both spots now
instead of calling symengine directly.

* Remove unused imports

* Gracefully handle failure to parse historical symengine files

During the discovery on the fix in this PR we discovered that setting
the ``use_symengine`` flag from Qiskit 0.45.x and 0.46.x would result in
newer versions of Qiskit being unable to parse the QPY file for the same
issue as being addressed here. This commit expands the logic to account
for this and raise a useful warning. It also updates the release notes
and documentation to document these limitations.

* Cap upper version of symengine in requirements list

Out of an abundance of caution this commit places a cap on the allowed
version of symengine users can install to be compatible with Qiskit.
Due to the symengine version dependence discovered in QPY around
serializing ParameterExpressions, we'll likely have a similar issue
when symengine 0.14.0 releases. Pre-emptively capping this means we
aren't going to be in this situation until we can confirm compatibility
with QPY serialization. The real solution for this will come in Qiskit#13252,
although as this behavior is embedded in QPY formats 10, 11, and 12 at
this point we'll have to handle this edge case moving forward regardless
of whether we introduce a better solution in 1.3.0 or not. Although
realistically in that case we will likely need to just document this as
a limitation when exporting QPY payloads with Qiskit 0.45.0 through
1.2.3 (and with the ``version`` flag set to >= 10 and < 13) and have
explicit error checking around the symengine version (which this PR
adds) when in that code path.

* Fix release note upgrade section label

* Rewrite support documentation

* Fix mistakes in release note

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: Jake Lishman <jake@binhbar.com>
mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Oct 22, 2024
With the release of symengine 0.13.0 we discovered a version dependence
on the payload format used for serializing symengine expressions. This
was worked around in Qiskit#13251 but this is not a sustainable solution and
only works for symengine 0.11.0 and 0.13.0 (there was no 0.12.0). While
there was always the option to use sympy to serialize the underlying
symbolic expression (there is a `use_symengine` flag on `qpy.dumps` you
can set to `False` to do this) the sympy serialzation has several
tradeoffs most importantly is much higher runtime overhead. To solve
the issue moving forward a qiskit native representation of the parameter
expression object is necessary for serialization.

This commit bumps the QPY format version to 13 and adds a new
serialization format for ParameterExpression objects. This new format
is a serialization of the API calls made to ParameterExpression that
resulted in the creation of the underlying object. To facilitate this
the ParameterExpression class is expanded to store an internal "replay"
record of the API calls used to construct the ParameterExpression
object. This internal list is what gets serialized by QPY and then on
deserialization the "replay" is replayed to reconstruct the expression
object. This is a different approach to the previous QPY representations
of the ParameterExpression objects which instead represented the internal
state stored in the ParameterExpression object with the symbolic
expression from symengine (or a sympy copy of the expression). Doing
this directly in Qiskit isn't viable though because symengine's internal
expression tree is not exposed to Python directly. There isn't any
method (private or public) to walk the expression tree to construct
a serialization format based off of it. Converting symengine to a sympy
expression and then using sympy's API to walk the expression tree is a
possibility but that would tie us to sympy which would be problematic
for Qiskit#13267 and Qiskit#13131, have significant runtime overhead, and it would
be just easier to rely on sympy's native serialization tools.

The tradeoff with this approach is that it does increase the memory
overhead of the `ParameterExpression` class because for each element
in the expression we have to store a record of it. Depending on the
depth of the expression tree this also could be a lot larger than
symengine's internal representation as we store the raw api calls made
to create the ParameterExpression but symengine is likely simplifying
it's internal representation as it builds it out. But I personally think
this tradeoff is worthwhile as it ties the serialization format to the
Qiskit objects instead of relying on a 3rd party library. This also
gives us the flexibility of changing the internal symbolic expression
library internally in the future if we decide to stop using symengine
at any point.

Fixes Qiskit#13252
github-merge-queue bot pushed a commit that referenced this issue Nov 6, 2024
* Add Qiskit native QPY ParameterExpression serialization

With the release of symengine 0.13.0 we discovered a version dependence
on the payload format used for serializing symengine expressions. This
was worked around in #13251 but this is not a sustainable solution and
only works for symengine 0.11.0 and 0.13.0 (there was no 0.12.0). While
there was always the option to use sympy to serialize the underlying
symbolic expression (there is a `use_symengine` flag on `qpy.dumps` you
can set to `False` to do this) the sympy serialzation has several
tradeoffs most importantly is much higher runtime overhead. To solve
the issue moving forward a qiskit native representation of the parameter
expression object is necessary for serialization.

This commit bumps the QPY format version to 13 and adds a new
serialization format for ParameterExpression objects. This new format
is a serialization of the API calls made to ParameterExpression that
resulted in the creation of the underlying object. To facilitate this
the ParameterExpression class is expanded to store an internal "replay"
record of the API calls used to construct the ParameterExpression
object. This internal list is what gets serialized by QPY and then on
deserialization the "replay" is replayed to reconstruct the expression
object. This is a different approach to the previous QPY representations
of the ParameterExpression objects which instead represented the internal
state stored in the ParameterExpression object with the symbolic
expression from symengine (or a sympy copy of the expression). Doing
this directly in Qiskit isn't viable though because symengine's internal
expression tree is not exposed to Python directly. There isn't any
method (private or public) to walk the expression tree to construct
a serialization format based off of it. Converting symengine to a sympy
expression and then using sympy's API to walk the expression tree is a
possibility but that would tie us to sympy which would be problematic
for #13267 and #13131, have significant runtime overhead, and it would
be just easier to rely on sympy's native serialization tools.

The tradeoff with this approach is that it does increase the memory
overhead of the `ParameterExpression` class because for each element
in the expression we have to store a record of it. Depending on the
depth of the expression tree this also could be a lot larger than
symengine's internal representation as we store the raw api calls made
to create the ParameterExpression but symengine is likely simplifying
it's internal representation as it builds it out. But I personally think
this tradeoff is worthwhile as it ties the serialization format to the
Qiskit objects instead of relying on a 3rd party library. This also
gives us the flexibility of changing the internal symbolic expression
library internally in the future if we decide to stop using symengine
at any point.

Fixes #13252

* Remove stray comment

* Add format documentation

* Add release note

* Add test and fix some issues with recursive expressions

* Add int type for operands

* Add dedicated subs test

* Pivot to stack based postfix/rpn deserialization

This commit changes how the deserialization works to use a postfix
stack based approach. Operands are push on the stack and then popped off
based on the operation being run. The result of the operation is then
pushed on the stack. This handles nested objects much more cleanly than
the recursion based approach because we just keep pushing on the stack
instead of recursing, making the accounting much simpler. After the
expression payload is finished being processed there will be a single
value on the stack and that is returned as the final expression.

* Apply suggestions from code review

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

* Change DERIV to GRAD

* Change side kwarg to r_side

* Change all the v4s to v13s

* Correctly handle non-commutative operations

This commit fixes a bug with handling the operand order of subtraction,
division, and exponentiation. These operations are not commutative but
the qpy deserialization code was treating them as such. So in cases
where the argument order was reversed qpy was trying to flip the
operands around for code simplicity and this would result in incorrect
behavior. This commit fixes this by adding explicit op codes for the
reversed sub, div, and pow and preserving the operand order correctly
in these cases.

* Fix lint

---------

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod: qpy Related to QPY serialization priority: high type: feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant