-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Introducing ScalableSymbolicPulse to correctly compare pulses of amp,angle representation #9314
Conversation
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:
|
Pull Request Test Coverage Report for Build 3896155519
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, adding new subclass seems better than assuming a generic logic. Thanks for proposing new implementation with this PR @TsafrirA .
The implementation looks bit complicated. Current implementation seems almost identical to do
class SymbolicPulse:
def __init__(self, ..., is_scalable=True)
without defining a subclass. To be more explicit, I think the constructor of the subclass can take amp
and angle
(as duration
has a dedicated role), and should full-scratch own equality logic.
For QPY, I think it's better to add class name *.__class__.__name__
in the SYMBOLIC_PULSE_PACK_V2
structure, rather than adding is_scalable
property. In future we may add other subclass with different set of parameter names with assigned role. So class name is much flexible (and we can avoid defining the SYMBOLIC_PULSE_PACK_V3
). I'm fine with bumping the QPY version in this situation.
This implementation looks nice to me. I will let @nkanazawa1989 provide the approval. I just want to mention a couple things about larger implications.
What kind of future pulse classes are you thinking of? If they implement features mutually exclusive to The other point I want to mention is that we should review when we need to check pulse equality. The main use cases that I can think of are the ones I mentioned in my inline comment about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @TsafrirA recent updates seem reasonable to me, but I also think revisiting the demand for hash is good idea. As @wshanks pointed out this is currently only used to validate amplitude (to avoid calling computationally heavy .get_waveform
as much as possible).
If we don't have further motivation for hashing (I think no), we can change _is_amplitude_valid
to directly call this line and let it take envelope and parameters which are both hashable.
https://github.com/Qiskit/qiskit-terra/blob/2f5944d60140ceb6e30d5b891e8ffec735247ce9/qiskit/pulse/library/symbolic_pulses.py#L490
I don't have any concrete idea in my mind, but I was just on the safe side. The original implementation introduced a boolean value (in the QPY data structure) to distinguish two classes. If we introduce another subclass in future PR we would need to bump the QPY version again and permanently maintain the another legacy loader. If we save class name instead of boolean we don't need to introduce such complexity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like new logic for the amplitude validation. Making pulses immutable looks right approach since they can be parameterized. This is almost good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like new logic for the amplitude validation. Making pulses immutable looks right approach since they can be parameterized. This is almost good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the removal of __hash__
as the path forward. I noted a few small things.
There was a problem hiding this 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 Does one of us put the automerge tag on this or do we let someone from the core team do that? I have this open in a browser tab and each time I pass by it I see it out of date with main and still re-running the checks, so I am not sure a human will ever get a chance to merge it 🙂 |
For reference: Will and I talked offline, and the answer is that code-owners are completely allowed (encouraged, even!) to apply "automerge" themselves when something's appropriate to merge. It's much better to use the bot than to do it manually because the bot will only update the PR when it's its turn to merge, avoiding unnecessary CI usage to test updates that'll never be up-to-date. |
…angle representation (Qiskit#9314) * Introduced ScalableSymbolicPulses to correctly compare pulses with amp,angle representation. * Modified ScalableSymbolicPulse, bumped QPY version to 6. * Bug fix * Documentation * Release Notes * Release Notes * Release Notes * Release Notes * Release Notes * Hash correction * Resolve GaussianSquareDrag conflict. * Resolve GaussianSquareDrag conflict. * Resolve GaussianSquareDrag conflict. * GaussianSquareDrag conversion to scalable and minor fixes. * added _read_symbolic_pulse_v6, and updated _loads_operand * Added ScalableSymbolicPulse to init * Minor fixes * Documentation fix * Minor fix to amplitude validation and release notes. * Add epsilon to amp validation. Co-authored-by: Naoki Kanazawa <[email protected]> Co-authored-by: Will Shanks <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Thanks Will for adding the automerge tag! I was waiting for your final review this time (if you wanted). |
I might as well mention for @nkanazawa1989 and @TsafrirA that Jake also pointed out that the reno file uses reStructuredText format rather than markdown, so for example in line code formatting should use two |
Thanks for the heads up @wshanks . This one's on me. |
Summary
Qiskit-Terra PR #9002, converted the library symbolic pulses from a complex
amp
representation to a real (float)amp
,angle
representation. To overcome the non-unique nature of the new representation (particularly, when negative amp is allowed), this PR introduces a new subclassScalableSymbolicPulse
which assumes theamp,angle
representation, and compares the pulses accordingly.PR #9257 presented a more general solution for any non-unique representation, but it was debated whether such a general solution is in fact needed.
Details and comments
Qiskit-Terra PR #9002, converted the library symbolic pulses from a complex
amp
representation to a real (float)amp
,angle
representation. To better support several experiments in Qiskit-Experiments, theamp
parameter is allowed to take both positive and negative values. This decision creates a non-unique representation for the pulses, where a pi shift ofangle
and a sign flip ofamp
have the same effect (the issue obviously also exists for 2*pi shift ofangle
). For example, the following two pulsesare not the same when equated (because their parameters are different):
but they represent the same waveform:
This PR sets to correct this situation, by introducing a new subclass
ScalableSymbolicPulse
. This class assumes that the pulse envelope has the formamp * exp(1j * angle)*F(t,parameters)
whereF
is some function. When two instances of this class are equated, the complex amplitudeamp * exp(1j * angle)
is compared, but the individual parametersamp,angle
are ignored.The comparison is being done using numpy.isclose(), with default values of rtol=1.e-5, atol=1.e-8, which I think is sufficient**.
the comparison of the complex amplitude (and the hashing) is carried out after a rounding to 6 decimal places. Considering the expected scale of the complex amplitude, and HW finite precision, this value should be sufficient to distinguish pulses which are really different from one another.
To accommodate the change, QPY format now includes a
class_name
field, which indicates the correct class when loading the pulse. QPY version was bumped to 6 to reflect this. QPY files of older versions assign only library pulses to the new subclass (even if a custom pulse might fit its purpose).Returning to the example above, the new PR yields:
Along the way, the issue of hashing was raised. Because equal objects should have the same hash, this made hashing very problematic (the only sensible solution was to bin the complex amplitude via rounding). To avoid this issue, and because
SymbolicPulse
hashing was only used for_is_amplitude_valid
, it was decided to remove the hashing option forSymbolicPulse
andScalableSymbolicPulse
. Note that the caching of_is_amplitude_valid
is based on a hashing which doesn't take into account the complex amp comparison.