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

Fix pulse parameter formatter bug with symengine #7265

Merged
merged 3 commits into from
Nov 17, 2021

Conversation

nkanazawa1989
Copy link
Contributor

Summary

Old formatter logic assumes always evaluated complex value is input, however, with symengine a parameter expression remains unevaluated though values are assigned. This usually crashes string-based complex value evaluation.

Details and comments

Use direct typecast instead of string-based evaluation. Truncation with the default of 10 decimal point is added to eliminate truncation error during typecast.

@coveralls
Copy link

coveralls commented Nov 15, 2021

Pull Request Test Coverage Report for Build 1473088604

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 82.549%

Totals Coverage Status
Change from base Build 1468573388: 0.001%
Covered Lines: 49833
Relevant Lines: 60368

💛 - Coveralls

@nkanazawa1989 nkanazawa1989 added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Nov 15, 2021
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, makes sense that since we now have a native __complex__ for ParameterExpressions that we should just leverage that directly. One question inline, but otherwise lgtm.

return evaluated
except ValueError:
except TypeError:
Copy link
Member

Choose a reason for hiding this comment

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

Do both symengine and sympy raise a type error here? I assume so since the unit tests pass, but just want to confirm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, locally I uninstalled symengine and it worked. Note that symengine and sympry don't raise errors. This is exception for
https://github.com/Qiskit/qiskit-terra/blob/277aa87f81a21d7516652051d6c615d18b291ea2/qiskit/circuit/parameterexpression.py#L442-L445

@mergify mergify bot merged commit ad75295 into Qiskit:main Nov 17, 2021
@nkanazawa1989 nkanazawa1989 deleted the fix-pulse-value-format-bug branch November 25, 2022 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants