Add ParameterExpression.numeric to cast to number#11109
Conversation
jakelishman
added
Changelog: Added
|
One or more of the the following people are requested to review this:
|
This is largely a wrap-up of the `if ParameterExpression.is_real(): ...` logic present in a few places across the library, but a bit more efficient for Symengine because it can re-use cases where it needed to force an evaluation to produce accurate results. Mostly, though, it simplifies a few call sites.
jakelishman
force-pushed
the
parameterexpression-numeric
branch
from
b3b0a4d to
1c023bd
Compare
Pull Request Test Coverage Report for Build 7259881506Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
1ucian0
left a comment
1ucian0
left a comment
There was a problem hiding this comment.
Looks good to me! Just some comments here and there.
| self.assertIsInstance(one_imaginary.numeric(), complex) | ||
| self.assertEqual(one_imaginary.numeric(), 1j) | ||
|
|
||
| # This is one particular case where symengine 0.9.2 (and probably others) struggles when |
There was a problem hiding this comment.
I'd leave it in regardless - it still works as a regression test for us.
| # Symengine returns `complex` if any imaginary floating-point enters at all, even if | ||
| # the result is zero. The best we can do is detect that and decay to a float. | ||
| out = complex(self._symbol_expr) | ||
| return out.real if out.imag == 0.0 else out |
There was a problem hiding this comment.
should this be a math.isclose? Zero-checking floats is always hard to me...
There was a problem hiding this comment.
Personally I'd say no - the only times we've seen uses where bad imaginary components enter is from bugs within Symengine itself (which this function works around), where the coefficient scales with the operation being done, so could be arbitrarily large. Auto clipping to zero also makes it tricky for people who were expecting to deal with very small numbers (which floating point is good at).
As an interface thing, a user can always clip the output themselves if needed, so I think not clipping is the safer choice.
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
5 participants
Summary
This is largely a wrap-up of the
if ParameterExpression.is_real(): ...logic present in a few places across the library, but a bit more efficient for Symengine because it can re-use cases where it needed to force an evaluation to produce accurate results. Mostly, though, it simplifies a few call sites.Details and comments
Depends on #11107, because
.evalf(real=True)is a symengine-specific extension, and whenTemplateOptimizationproduces mismatchedParameterExpressions it breaks this handling.The additional handling in
numericto reduce the impact of the spurious imaginary components makes this generally more reliable. I came across problems with global phases during binding in a PR built on top of this - at the moment, the type of theglobal_phaseinQuantumCircuit.assign_parametersisn't validated in the same way, but in a unification PR I had it was, and some deeply nested (but completely valid) assignments were failing.