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 disable_validation option to SymbolicPulse #11029

Merged
merged 4 commits into from
Oct 23, 2023

Conversation

TsafrirA
Copy link
Collaborator

@TsafrirA TsafrirA commented Oct 17, 2023

Summary

A disable_validation option was added to the SymbolicPulse class. This option improves the JAX compatibility of Qiskit Pulse, which is important for Qiskit Dynamics users.

Background

Qiskit Dynamics relies on JAX to massively improve run times. However, this improvement comes at a cost - the code has to be JAX compatible, and mundane operations become problematic. The parameter validation of SymbolicPulse objects falls under this problematic category and limits the ability to use Qiskit Pulse with Qiskit Dynamics. For example, this Qiskit Dynamics tutorial defines from scratch a Gaussian pulse, simply to avoid the parameter validation.

Details and comments

A disable_validation option was added to SymbolicPulse class. The option can be set the class attribute disable_validation. For example, the following pulse will trigger the validation automatically and fail it:

import qiskit.pulse as pulse

pulse.Constant(duration=100, amp=1.4)

however, this will not:

import qiskit.pulse as pulse

pulse.SymbolicPulse.disable_validation = True
pulse.Constant(duration=100, amp=1.4)

The triggering of the validate_parameters method was moved from individual library pulses, directly into the instantiation of SymbolicPulse object. As all library pulses triggered the validation, this will not have any effect on them. However, user defined SymbolicPulsess which did not trigger the validation, will now trigger it, potentially causing a disruption. It will be an easy fix though.

With the Qiskit Pulse compiler coming along (#10759 ) it was likely that the validation would be delegated to the compiler anyway. In that sense, disabling the validation is not far off from what we are going for anyway.

I've taken the liberty to remove some limit_amplitude tests, which were redundant.

@TsafrirA TsafrirA requested review from eggerdj, wshanks and a team as code owners October 17, 2023 13:06
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core
  • @nkanazawa1989

@coveralls
Copy link

coveralls commented Oct 17, 2023

Pull Request Test Coverage Report for Build 6602984928

  • 17 of 17 (100.0%) changed or added relevant lines in 1 file are covered.
  • 19 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.02%) to 86.871%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 1 91.92%
crates/qasm2/src/parse.rs 18 95.74%
Totals Coverage Status
Change from base Build 6594932071: -0.02%
Covered Lines: 73829
Relevant Lines: 84987

💛 - Coveralls

@nkanazawa1989 nkanazawa1989 added the mod: pulse Related to the Pulse module label Oct 17, 2023
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.

@TsafrirA Sorry about slow response. My main concern here is having a class variable in the constructor. Could you please reconsider? Also I need new-feature reno to approve.

qiskit/pulse/library/symbolic_pulses.py Outdated Show resolved Hide resolved
@@ -390,6 +390,8 @@ def Sawtooth(duration, amp, freq, name):
"_valid_amp_conditions",
)

disable_validation = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Since argument is called constraints, something like ignore_constraints is more intuitive?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nkanazawa1989
The function is called validate_parameters so I went with that. Note that the function also validates the amplitude, and not just the constraints.

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 @TsafrirA this looks good to me.

@nkanazawa1989 nkanazawa1989 added this pull request to the merge queue Oct 23, 2023
Merged via the queue into Qiskit:main with commit 5de1a06 Oct 23, 2023
14 checks passed
@TsafrirA TsafrirA deleted the DisablePulseValidation branch October 23, 2023 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod: pulse Related to the Pulse module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants