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

Remove complex amp support for ScalableSymbolicPulse #11403

Merged
merged 11 commits into from
Jan 15, 2024

Conversation

TsafrirA
Copy link
Collaborator

Summary

Support for complex amp in the instantiation of ScalableSymbolicPulse is removed.

Details and comments

Complex amp support was deprecated in #10357 (with some parts awaiting deprecation with #11257 in the 0.46 branch). This PR removes the support.

Pulses with complex amp loaded from old QPY files are automatically converted to (amp,angle) representation (with a user warning). Note that there are three eras with regard to this - complex amp (Terra <0.23), amp,angle with complex amp support (Qiskit < 1.0) and now amp,angle with no complex amp support (Qiskit >= 1.0). QPY version was not bumped, so the "complexity" of amp is tested even for QPY files of Qiskit >=1.0. With the next version of QPY, amp could be assumed to be real.

The QPY compatibility test was updated to reflect the difference in the API, but backwards compatibility is maintained.

@TsafrirA TsafrirA requested review from eggerdj, wshanks and a team as code owners December 12, 2023 13:14
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core
  • @mtreinish
  • @nkanazawa1989

@coveralls
Copy link

coveralls commented Dec 12, 2023

Pull Request Test Coverage Report for Build 7480324698

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.003%) to 87.595%

Totals Coverage Status
Change from base Build 7476756860: -0.003%
Covered Lines: 59655
Relevant Lines: 68103

💛 - Coveralls

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

Thanks @TsafrirA for taking care of this :)

ref_obj = pulse.Constant(duration=160, amp=1j * 0.1)

self.assertEqual(assigned, ref_obj)
self.assertEqual(assigned.amp, 0.1j)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this is confusing because we can still use complex valued amplitude through parameterization (because this bypasses validation in the constructor). We can probably drop validation in the constructor and let user use valid amplitude value on their responsibility. Note that this pulse will fail in execution anyways because QPY doesn't take complex value, right? I think this test doesn't really make sense.

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

With Qiskit's main branch you can save a ScheduleBlock with complex amp.

In #9897 we added a PendingDeprecationWarning for the assignment of complex values. We can promote to deprecation in 0.46 and remove in 1.0 if you want.

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.

LGTM. Just minor fix. Qiskit 1.0 should not contain deprecation warning and I think we need to drop support for parameter assignment of complex values.

qiskit/qpy/binary_io/schedules.py Outdated Show resolved Hide resolved
qiskit/qpy/binary_io/schedules.py Outdated Show resolved Hide resolved
TsafrirA and others added 2 commits January 10, 2024 10:54
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
@TsafrirA
Copy link
Collaborator Author

Thanks @nkanazawa1989 !
I'll take a look at the parameter assignment. I hope I can squeeze it in before the deadlines.

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.

LGTM, thanks!

@nkanazawa1989 nkanazawa1989 added this pull request to the merge queue Jan 15, 2024
Merged via the queue into Qiskit:main with commit 970f1f5 Jan 15, 2024
13 checks passed
ShellyGarion pushed a commit to ShellyGarion/qiskit-terra that referenced this pull request Jan 18, 2024
* Remove complex amp support

* QPY compatibility fix

* lint

* Yet another QPY compatibility fix

* Yet another QPY compatibility fix

* Yet another QPY compatibility fix

* Update qiskit/qpy/binary_io/schedules.py

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

* Update qiskit/qpy/binary_io/schedules.py

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

* lynt

---------

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
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