Skip to content

Improve casting and error message for ParameterExpression #10244

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 12, 2023
Merged

Improve casting and error message for ParameterExpression #10244

merged 3 commits into from
Jun 12, 2023

Conversation

jakelishman
Copy link
Member

Summary

Previously, we assumed that the only reason a cast of ParameterExpression to complex/float/int could fail was because of unbound parameters, and emitted an error accordingly. This is not the case; a fully bound complex value will still fail to convert to float.

This PR improves the error messages in these cases, and works around a difference of Sympy and Symengine, where the latter will fail to convert real-valued expressions that were symbollically complex at some point in their binding history to float. Sympy more reliably reduces values down to real-only values when the imaginary part is exactly cancelled, which is a use-case our users tend to expect.

Details and comments

Fix #9187
Fix #10191

@wshanks: could I ask you to test this with one of your previous failing examples, just to check any regression? I think we included all the relevant parts in our test suite, but I don't want to accidentally regress for you.

@jakelishman
Improve casting and error message for ParameterExpression
953bdd3 
Previously, we assumed that the only reason a cast of
`ParameterExpression` to `complex`/`float`/`int` could fail was because
of unbound parameters, and emitted an error accordingly.  This is not
the case; a fully bound complex value will still fail to convert to
`float`.

This PR improves the error messages in these cases, and works around a
difference of Sympy and Symengine, where the latter will fail to convert
real-valued expressions that were symbollically complex at some point in
their binding history to `float`.  Sympy more reliably reduces values
down to real-only values when the imaginary part is exactly cancelled,
which is a use-case our users tend to expect.
@jakelishman jakelishman added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog labels Jun 8, 2023
@jakelishman jakelishman added this to the 0.24.2 milestone Jun 8, 2023
@jakelishman jakelishman requested a review from a team as a code owner June 8, 2023 13:17
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Jun 8, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 5213303912

  • 13 of 17 (76.47%) changed or added relevant lines in 2 files are covered.
  • 7 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.06%) to 85.905%
Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/parameterexpression.py 11 15 73.33%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/sabre_swap/mod.rs 1 99.77%
crates/qasm2/src/expr.rs 1 93.76%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 1 90.18%
crates/qasm2/src/lex.rs 2 90.89%
qiskit/extensions/quantum_initializer/squ.py 2 80.0%
Totals Coverage Status
Change from base Build 5201330574: 0.06%
Covered Lines: 71264
Relevant Lines: 82957
💛 - Coveralls

kevinhartman
kevinhartman reviewed Jun 8, 2023
View reviewed changes
Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once we get validation from wshanks.

wshanks
wshanks approved these changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@wshanks wshanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. In working on #10183, I did not personally have a case where a complex expression needed to evaluate back to a real value. Where I ran into that problem was that my initial change caused this test to fail until I put in a workaround:

https://github.com/jakelishman/qiskit-terra/blob/3e5770148e92698418a292b552e1d15d6196bce9/test/python/circuit/test_parameters.py#L1442-L1461

Since that test is still passing here, I think the change here is good. I checked that the example in #9187 also ran without error.

@jakelishman
Copy link
Member Author

Thanks Will.

I'm personally torn on things like float((0.5j * x).assign({x: 0.5j})) working. I don't like it because the complex number is explicitly made a float not a symbol, and float(1 + 0j) is a type error because the general conversion is fallible. That said, I think users of this class all assume that x.is_real and not x.parameters implies that float(x) should succeed, and there would be little way to test for success other than try/except if we broke that assumption. Sympy also more aggressively reassigns the internal type back down to a real expression than Symengine, so it's better to be consistent as well. On that basis (and because you're already obviously relying on it!) I think it's best to keep it, it just won't be my favourite corner of Terra.

@jakelishman jakelishman requested a review from kevinhartman June 12, 2023 15:56
kevinhartman
kevinhartman approved these changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kevinhartman kevinhartman added this pull request to the merge queue Jun 12, 2023
Merged via the queue into Qiskit:main with commit 86df15d Jun 12, 2023
mergify bot pushed a commit that referenced this pull request Jun 12, 2023
@jakelishman @mergify
Improve casting and error message for ParameterExpression (#10244)
7a3512b 
* Improve casting and error message for `ParameterExpression`

Previously, we assumed that the only reason a cast of
`ParameterExpression` to `complex`/`float`/`int` could fail was because
of unbound parameters, and emitted an error accordingly.  This is not
the case; a fully bound complex value will still fail to convert to
`float`.

This PR improves the error messages in these cases, and works around a
difference of Sympy and Symengine, where the latter will fail to convert
real-valued expressions that were symbollically complex at some point in
their binding history to `float`.  Sympy more reliably reduces values
down to real-only values when the imaginary part is exactly cancelled,
which is a use-case our users tend to expect.

* Fix typo in test

* Update releasenotes/notes/parameter-float-cast-48f3731fec5e47cd.yaml

Co-authored-by: Kevin Hartman <kevin@hart.mn>

---------

Co-authored-by: Kevin Hartman <kevin@hart.mn>
(cherry picked from commit 86df15d)

# Conflicts:
#	test/python/circuit/test_parameters.py
@mergify mergify bot mentioned this pull request Jun 12, 2023
Merged
jakelishman added a commit that referenced this pull request Jun 15, 2023
@jakelishman @kevinhartman
Improve casting and error message for ParameterExpression (#10244)
b988969 
* Improve casting and error message for `ParameterExpression`

Previously, we assumed that the only reason a cast of
`ParameterExpression` to `complex`/`float`/`int` could fail was because
of unbound parameters, and emitted an error accordingly.  This is not
the case; a fully bound complex value will still fail to convert to
`float`.

This PR improves the error messages in these cases, and works around a
difference of Sympy and Symengine, where the latter will fail to convert
real-valued expressions that were symbollically complex at some point in
their binding history to `float`.  Sympy more reliably reduces values
down to real-only values when the imaginary part is exactly cancelled,
which is a use-case our users tend to expect.

* Fix typo in test

* Update releasenotes/notes/parameter-float-cast-48f3731fec5e47cd.yaml

Co-authored-by: Kevin Hartman <kevin@hart.mn>

---------

Co-authored-by: Kevin Hartman <kevin@hart.mn>
jakelishman added a commit that referenced this pull request Jun 16, 2023
@mergify @jakelishman @kevinhartman
Improve casting and error message for ParameterExpression (#10244) (#…
31f26d2 
…10268)

* Improve casting and error message for `ParameterExpression`

Previously, we assumed that the only reason a cast of
`ParameterExpression` to `complex`/`float`/`int` could fail was because
of unbound parameters, and emitted an error accordingly.  This is not
the case; a fully bound complex value will still fail to convert to
`float`.

This PR improves the error messages in these cases, and works around a
difference of Sympy and Symengine, where the latter will fail to convert
real-valued expressions that were symbollically complex at some point in
their binding history to `float`.  Sympy more reliably reduces values
down to real-only values when the imaginary part is exactly cancelled,
which is a use-case our users tend to expect.

* Fix typo in test

* Update releasenotes/notes/parameter-float-cast-48f3731fec5e47cd.yaml



---------

Co-authored-by: Jake Lishman <jake@binhbar.com>
Co-authored-by: Kevin Hartman <kevin@hart.mn>
thspreetham98 pushed a commit to thspreetham98/qiskit-terra that referenced this pull request Jun 19, 2023
@jakelishman @kevinhartman @thspreetham98
Improve casting and error message for ParameterExpression (Qiskit#1…
590bdd0 
…0244)

* Improve casting and error message for `ParameterExpression`

Previously, we assumed that the only reason a cast of
`ParameterExpression` to `complex`/`float`/`int` could fail was because
of unbound parameters, and emitted an error accordingly.  This is not
the case; a fully bound complex value will still fail to convert to
`float`.

This PR improves the error messages in these cases, and works around a
difference of Sympy and Symengine, where the latter will fail to convert
real-valued expressions that were symbollically complex at some point in
their binding history to `float`.  Sympy more reliably reduces values
down to real-only values when the imaginary part is exactly cancelled,
which is a use-case our users tend to expect.

* Fix typo in test

* Update releasenotes/notes/parameter-float-cast-48f3731fec5e47cd.yaml

Co-authored-by: Kevin Hartman <kevin@hart.mn>

---------

Co-authored-by: Kevin Hartman <kevin@hart.mn>
@jakelishman jakelishman deleted the parameter-float-cast branch June 20, 2023 11:45
@Raghav-Bell Raghav-Bell mentioned this pull request Aug 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wshanks wshanks wshanks approved these changes

@kevinhartman kevinhartman kevinhartman approved these changes

Assignees
No one assigned
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Milestone
0.24.2
5 participants
@jakelishman @qiskit-bot @coveralls @wshanks @kevinhartman