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

Make numeric casts of ParameterExpression more flexible #6802

Merged

Conversation

jakelishman
Copy link
Member

Summary

Previously, ParameterExpression would refuse numeric conversions if it
contained unbound parameters, even if the underlying symbolic expression
had a known, fixed value. This patch passes the entire responsibility
for determining if a numeric conversion is possible to Sympy/Symengine,
meaning that expressions such as

>>> x = Parameter('x')
>>> float(x - x + 2)
2.0

are now possible.

Details and comments

Follows on from comments in this chain: #6792 (comment).

An alternative approach is to avoid the try/except by querying if _symbol_expr.expr_free_symbols is empty, and raise if not. I'm not 100% sure this is the only thing that can cause a numeric conversion in Sympy to fail though, so I left it on the "safe" side for now.

@jakelishman jakelishman requested a review from a team as a code owner July 26, 2021 14:20
@jakelishman jakelishman added the Changelog: New Feature Include in the "Added" section of the changelog label Jul 26, 2021
@Cryoris
Copy link
Contributor

Cryoris commented Jul 28, 2021

That's a very useful fix 👍🏻

Another way to resolve this issue (and also close #6812 along the way) could be to make sure the arithmetic operations on parameters (like x - x) check what parameters are actually left in the result. That's a bit more work, but then we would also improve cases like

print((x - x).parameters())  # currently: {x}, but should actually be the empty set

but if this is a pressing issue we can also merge this first and follow up 🙂

@jakelishman
Copy link
Member Author

Yeah, what you're saying is the behaviour I'd have expected. I wasn't sure if the behaviour you've metioned is used for anything interally - I didn't know if there was a reason for ParameterExpression to track parameters that it used to have in addition to ones it currently has.

@Cryoris
Copy link
Contributor

Cryoris commented Jul 28, 2021

Since the arithmetic ops return new instances I don't think it needs to track which parameters it had, the current ones should be enough 🤔

@dlasecki
Copy link
Contributor

dlasecki commented Oct 7, 2021

Hi, what is the timeline for merging this PR?

@jakelishman
Copy link
Member Author

I had completely forgotten about it, to be honest. I'll try and have a look again, and look into closing #6812 within the next week, with an aim to land it in Terra 0.19.

Previously, ParameterExpression would refuse numeric conversions if it
contained unbound parameters, even if the underlying symbolic expression
had a known, fixed value.  This patch passes the entire responsibility
for determining if a numeric conversion is possible to Sympy/Symengine,
meaning that expressions such as

    >>> x = Parameter('x')
    >>> float(x - x + 2)
    2.0

are now possible.
@jakelishman jakelishman force-pushed the numeric-conversions-ParameterExpression branch from d65559c to d859090 Compare October 14, 2021 12:14
@jakelishman
Copy link
Member Author

I've rebased this over the recent changes to EvolvedOperatorAnsatz and NLocal, but I haven't put in a solution to #6812 for this PR. That's a bit of a bigger change - we currently don't have the necessary directional mappings in ParameterExpression to do this efficiently, and adding them will have some knock-on repercussions, which might be better served being done in a separate PR.

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

About my earlier comment: automatically removing variables if the expression contains e.g. x - x requires a bit more care to make sure they aren't removed from the circuit and binding them fails. So this is a good solution I think.

@mergify mergify bot merged commit 7a13cc8 into Qiskit:main Nov 19, 2021
@jakelishman jakelishman deleted the numeric-conversions-ParameterExpression branch November 27, 2021 00:05
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants