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 relational, min, max, floor, ceil, sqrt, arctan2 to ParameterExpression #5278

Closed
wants to merge 40 commits into from

Conversation

ewinston
Copy link
Contributor

@ewinston ewinston commented Oct 22, 2020

Summary

Adds relational expressions to ParameterExpression as well as floor, ceil, max, min, sqrt, arctan2, and, or, piecewise.

Details and comments

Depends on #4854
Resolves #5276
Resolves #6376

@ewinston ewinston requested a review from a team as a code owner October 22, 2020 18:15
@ewinston ewinston changed the title add relational, min, max, floor, ceil to ParameterExpression add relational, min, max, floor, ceil, sqrt, arctan2 to ParameterExpression Oct 23, 2020
qiskit/circuit/parameterexpression.py Outdated Show resolved Hide resolved
qiskit/circuit/parameterexpression.py Outdated Show resolved Hide resolved
qiskit/circuit/parameterexpression.py Outdated Show resolved Hide resolved
test/python/circuit/test_parameters.py Outdated Show resolved Hide resolved
qiskit/circuit/parameterexpression.py Outdated Show resolved Hide resolved
qiskit/circuit/parameterexpression.py Outdated Show resolved Hide resolved
test/python/circuit/test_parameters.py Outdated Show resolved Hide resolved
@eendebakpt
Copy link
Contributor

@ewinston Any update on this PR?

@peendebak
Copy link
Contributor

@ewinston @kdk What is the status of the PR?

@ewinston
Copy link
Contributor Author

ewinston commented Jul 7, 2021

Hi @peendebak this pr opens up broadened functionality of ParameterExpressions and there may be some concern about whether it is needed particularly when considering the desire to reduce dependence on sympy due to speed considerations. Do you have a use case you were considering?

@peendebak
Copy link
Contributor

@ewinston I have two use cases. Both involve ParameterExpressions which start symbolic, but at some point in the computation are just floats (but represented as a ParameterExpression). Then I want to perform either a comparison on the expression, or take the abs and then compare. For example here is the abs used: https://github.com/Qiskit/qiskit-terra/pull/6472/files#diff-ce3115c08c6064bff6d00bd1ec433fa5568dabf9db1f8dbb2979032147e09a00R50 . The other use case was (I think) a comparison to check whether a duration is strictly positive.

These cases do not require the full PR, but just the abs and comparison operators.

Both cases can be solved by first casting to float and then performing the operation, but this requires casts at various places and it is a little bit hard to predict when it is needed and when not.

Another option that keeps the ParameterExpression small and would (perhaps) improve performance would be to automatically convert a ParameterExpression without symbols to a float

from qiskit.circuit import Parameter

p=Parameter('s')
delta = p-p # is still a ParameterExpression, but could be a float
delta > 0 # currently fails, but would work if the result of p-p would be cast to float automatically

@jakelishman
Copy link
Member

This PR is old, and I think that incoming changes to how classical types are handled will render all of the relational parts anyway. I've laid out my reasoning in other places (e.g. #7497 (review)), but primarily I think that right now ParameterExpression is designed to represent real- and complex-valued expressions, not Boolean-valued relational statements. All the usage examples I have seen for these would not work with actual symbols, and there is no part of Qiskit that would actually make use of symbolic relational arguments. ParameterExpression doesn't have a type associated with it, so there would be no way for a user of ParameterExpression to know what sort of value was being represented.

The uses cited all seem to revolve around issues caused by ParameterExpression.bind not producing a float once it is fully bound, or our failure to ensure that inputs are numerical in other places in the code. I'm very strongly against attempting to patch ParameterExpression with more and more relational expressions to try and make this work; it's not what it's designed to be, and as best as I can see, it seems to be fixing the wrong problems.

Mathematical operations that return real values could be useful, but it doesn't seem like they're super pressing.

@ewinston
Copy link
Contributor Author

My use case for this was getting the RVGate to be in the standard_gates directory. I believe at the time that gates in that directory were required to take ParameterExpression arguments but the decomposition method we used needed something numeric. Using relationals and Piecewise I had a symbolic decomposition just about worked out (which exists in these tests) but abandoned it since it was just a "would be nice" sort of thing.

I'll close this now unless there is stronger justification in the future.

@ewinston ewinston closed this Jan 31, 2022
@ewinston ewinston deleted the paramufuncs branch April 8, 2022 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ParameterExpression does not support comparison operators Min, max, ceil, floor in ParameterExpression
6 participants