-
Notifications
You must be signed in to change notification settings - Fork 37
Making sure all half_pulse_durations are positive #68
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
Conversation
|
|
||
| if not np.isclose(operations[1, op_idx], 0.): # Rabi rotation | ||
| half_pulse_duration = 0.5 * max(operations[1, op_idx] / maximum_rabi_rate, | ||
| half_pulse_duration = 0.5 * max(np.abs(operations[1, op_idx]) / maximum_rabi_rate, |
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.
Assuming that previously if a negative rotation was provided we'd eventually throw an error (due to the segment ending before it started), I think we should continue to do that now (just with a more explicit check and error message). I'd be a bit scared taking an absolute value, since then we might be silently giving a user a result they didn't expect.
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.
Fair enough.
For what's worth, an error was still being thrown if the user selected a negative Rabi rotation, only further down the line, because DrivenControls doesn't accept negative Rabi rates. (Although the DDS class does accept negative Rabi rotations without throwing an error, which is a bit of a contradiction in my view.) In my opinion, the error that was being thrown (complaining explicitly about the wrong value of the Rabi rate) was more useful than the previous error (simply saying that something was wrong in creating a sequence and suggesting a change in the maximum_rabi_rate, which wouldn't help in this case).
It doesn't hurt to add an extra check, though, so now I added one that throws an exception before any Rabi rotation values are used
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.
Ah, OK, I didn't realise we'd throw an error anyway (and a useful error, at that). Still, in general I think it's best to throw as early as possible, and this way we still get to keep a useful error message.
Agree that it's inconsistent for DDS to accept negative Rabi rates -- maybe a future project could be to explicitly disallow them (or explicitly allow them, and make clear how they're interpreted). If that sounds reasonable I'll file a ticket.
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.
Sounds very reasonable to me!
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.
Filed https://q-ctrl.atlassian.net/browse/QENG-795. IMO not a high priority, so maybe we can start grouping together some "if we care about open controls then we should fix these" tickets.
This is the first step towards simplifying and making the argument check mentioned in the following comment more floating-point friendly:
#67 (comment)
in order to solve Luigi's bug.
This PR eliminates the need of checking whether all the pulse starts are smaller or equal to the pulse ends, by making sure that all the
half_pulse_durationare positive. This is done in two parts:Making sure we take the absolute value of the Rabi rotation when calculating the
half_pulse_duration. In principle this should be a positive value, but lacking any prior test there might be nothing stopping the user to try to set it as -pi/2 as a way to obtain a (-pi/2)-pulse in the X direction, for example. (The proper way to do that would be to set the Rabi rotation as pi/2 and the azimuthal angle as pi.)Making sure
maximum_rabi_rateandmaximum_detuning_rateare not zero, or we would be dividing by zero and obtaining infinite durations for the pulses. This is done by updating the_check_maximum_rotation_rateprivate function.After this, the
pulse_start_endsis only updated by adding the same translation value to the start and the end of the pulse, so there shouldn't be any case where the start and end points switch positions. The only two exceptions were:The first case is a logic contradiction, so it would never occur. I can't imagine why the second case is meaningful (if you have two pulses at the origin for a sequence of duration zero, the translation procedure should still work), so I've eliminated it as well.
Finally, I realized that if someone sets
maximum_detuning_rate = Nonethe_check_maximum_rotation_rate()fails, so the check forif maximum_detuning_rate is Noneis never activated, so I've eliminated it as well.