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

Add pow as supported operation for ParameterExpression #11235

Merged
merged 4 commits into from
Nov 29, 2023

Conversation

grossardt
Copy link
Contributor

Summary

Adds powering for qiskit.circuit.ParameterExpression allowing to use a**n, n**a, a**b, pow(a,b) etc. for ParameterExpressions a, b and numbers n.

Details and comments

Minimal change using default support of pow by Sympy/Symengine.
Also added pow to list of operators in TestParameterExpressions test case for unit testing.
Fixes #8959
Changelog: New Feature

Adds powering for qiskit.circuit.ParameterExpression allowing to use a**n, n**a, a**b, pow(a,b) etc. for ParameterExpressions a, b and numbers n. Minimal change using default support of pow by Sympy/Symengine.
Added pow to list of operators in TestParameterExpressions test case for unit testing.
fixes Qiskit#8959
Changelog: New Feature
@grossardt grossardt requested a review from a team as a code owner November 12, 2023 23:46
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Nov 12, 2023
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Nov 13, 2023

Pull Request Test Coverage Report for Build 6914056656

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 10 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.02%) to 85.91%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.76%
crates/qasm2/src/lex.rs 3 91.92%
crates/qasm2/src/parse.rs 6 97.6%
Totals Coverage Status
Change from base Build 6911016807: 0.02%
Covered Lines: 65947
Relevant Lines: 76763

💛 - Coveralls

@grossardt
Copy link
Contributor Author

It seems that the Windows version (of Symengine I assume) introduces a small numerical error in pow that leads to some of the tests to fail. Not sure how to deal with this? Just loosen the test to only assert almost equal?

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Hmm, this is a slightly tricksy question. On the one hand, I'm not keen to relax the tolerances of the test, because the expressions are a simple binary op, and the floating-point evaluation should be bit-for-bit identical between the two cases. On the other, complex-number powers are a mathematically tricky operation to perform, so they're not really akin to the simple +, -, * and / operations otherwise tested.

The failing test case is pow(-1, 2.3), which in pure real-number arithmetic should be equal to $e^{3i\pi/10} = \cos(3\pi/10) + i\sin(3\pi/10)$. The closest floats to those values would be (0.5877852522924731+0.8090169943749475j), which notably isn't the returned result from either component, so there's already a few-ULP difference in both pow(-1, 2.3) and the symbolic form. That's quite to be expected, since even one of the basic simplifications I did ($2.3 - 2 = 0.3$) is not exactly true in floating-point operations due to rounding in the [2, 4) interval being different to the [0.25, 0.5) interval: 2.3 - 2 != 0.3. That gets even more tricky when introducing pi, and that the power is eventually going to be evaluated by summing power series in some form or another.

I think preferentially I'd like to keep the exact bit-for-bit equality assertion for +, -, * and /, because those should be bit-for-bit equal. For power, perhaps it would be cleaner to introduce a new test for power. I don't mind too much if we want to make that use floating-point numbers that should produce bit-for-bit identical outputs, or if we want to relax the assertions to fuzzy matches with ~5 ULP tolerance (or whatever's appropriate).


The actual code and release note of this PR looks totally fine to me, thanks.

@grossardt
Copy link
Contributor Author

grossardt commented Nov 16, 2023

@jakelishman Sounds good. I will add new tests for pow then (probably this weekend). Will opt for the fuzzy matches (mainly because the issue doesn't appear on Linux and I have no Windows environment for testing, so this seems easier).

PS: it looks like there are no tests for exp, log, sin, ...? or am I missing those?

@grossardt
Copy link
Contributor Author

In TestParameterExpressions I changed supported_operations to a dictionary with values indicating tolerance for fuzzy comparison. This shouldn't break anything and leave the option to add more operations in the future.
I also used the opportunity to add a further test that asserts $x^y = exp(y log(x))$, hence testing both pow and exp, log methods (even though this mainly tests sympy functionality, that way exp, log get called at least once, which in my understanding they haven't - I was wrong about trigonometric functions which do have tests).
I hope the tests pass now (they did locally for me).

Copy link
Member

@jakelishman jakelishman 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 great, thanks for the new tests!

@jakelishman jakelishman added this pull request to the merge queue Nov 29, 2023
@jakelishman jakelishman added Changelog: New Feature Include in the "Added" section of the changelog mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Nov 29, 2023
@jakelishman jakelishman added this to the 1.0.0pre1 milestone Nov 29, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 29, 2023
@jakelishman jakelishman added this pull request to the merge queue Nov 29, 2023
Merged via the queue into Qiskit:main with commit b72033e Nov 29, 2023
14 checks passed
@grossardt grossardt deleted the add-parameter-pow-issue-8959 branch November 29, 2023 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Support powering Parameters
4 participants