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

Update GaussianPulse for consistent parameter validation results whether validation occurs during or after construction. #8151

Merged

Conversation

upsideon
Copy link
Contributor

@upsideon upsideon commented Jun 8, 2022

Summary

Fixes #7882 by moving a check for the simultaneously specification of width and risefall_sigma_ratio from qiskit.pulse.library.GaussianPulse.validate_parameters to the class constructor. This ensures that when the construction process internally sets risefall_sigma_ratio using the value of width, that subsequent validations, via calls to validate_parameters do not fail erroneously.

@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 Jun 8, 2022

Pull Request Test Coverage Report for Build 2488093915

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 84.424%

Totals Coverage Status
Change from base Build 2484981305: 0.0%
Covered Lines: 54712
Relevant Lines: 64806

💛 - Coveralls

Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Probably extra documentation is confusing since this is a public method. I prefer moving duplication check of (width, risefall_sigma_ratio) from .validate_parameters method to the GaussianSquare pulse constructor.

Same modification has been made in GaussianSquare of the SymbolicPulse (which is replacement of ParametricPulse). Details in #7821 (symbolic_pulses.py)

@1ucian0 1ucian0 added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 10, 2022
@upsideon
Copy link
Contributor Author

Thank you for the review, @nkanazawa1989! That is a good approach. I've applied the required code changes and added a test that would have caught this bug and will catch others like it, if they occur.

@upsideon upsideon changed the title Document that ParametricPulse.validate_parameters is for internal use only. Update GaussianPulse for consistent parameter validation results whether validation occurs during or after construction. Jun 10, 2022
@upsideon upsideon requested a review from nkanazawa1989 June 10, 2022 20:42
nkanazawa1989
nkanazawa1989 previously approved these changes Jun 12, 2022
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Thanks, this is nice bug fix :)

jakelishman
jakelishman previously approved these changes Jun 13, 2022
@jakelishman jakelishman added automerge Changelog: Bugfix Include in the "Fixed" section of the changelog labels Jun 13, 2022
@jakelishman jakelishman force-pushed the update-parametric-pulse-validate-parameters-docs branch from 02067f0 to fb24525 Compare June 13, 2022 11:42
@mergify mergify bot merged commit 669c771 into Qiskit:main Jun 13, 2022
@upsideon upsideon deleted the update-parametric-pulse-validate-parameters-docs branch June 13, 2022 13:57
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 Community PR PRs from contributors that are not 'members' of the Qiskit repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Method validate_parameter() of GaussianSquare raises PulseError if you call it on a created pulse
6 participants