-
Notifications
You must be signed in to change notification settings - Fork 157
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
Verify values of parametrized rzz gates #2021
base: main
Are you sure you want to change the base?
Conversation
Following #2035, we should also take into account parameter expressions. |
A couple of comments:
|
Co-authored-by: Takashi Imamichi <[email protected]>
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.
LGTM. But since I'm not an official approver, you need to ask someone else to review it too.
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.
LGTM, can you add a release note?
qiskit_ibm_runtime/utils/utils.py
Outdated
if isinstance(angle, Parameter): | ||
angle_params.add(angle.name) | ||
elif not isinstance(angle, ParameterExpression) and ( | ||
angle < 0.0 or angle > 1.001 * np.pi / 2 |
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.
There is an asymmetry here in that rounding errors are compensated on the upper end, but not the lower end. for example, -1e-12
is invalid whereas np.pi/2 + 1e-12
is valid.
I'm also a bit surprised that the level is at 1e-3, very far away from machine epsilon. For example, is there a problem moving this to 1e-10 which is still 100x machine epsilon?
angle < 0.0 or angle > 1.001 * np.pi / 2 | |
not -EPSILON < angle < np.pi / 2 + EPSILON |
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.
This mirrors line 82 above. I think I contributed to the original choice of 1.001. The hardware is optimized for pi/2 because the main focus is the cz gate. It doesn't blow up if you go just past pi/2 but you don't want to go much past it (not calibrated and will eventually hit some hardware/physical limit). So it is not critical that the limit be tight on pi/2 but whatever is likely not to run into rounding issues should be okay.
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.
Then, what's the decision, for both ends?
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.
Once there is a decision, I'll of course fix (if needed) not only here, but also in several other places where this check takes place (I've spotted that this check appears in three different places, maybe I'll write a tiny function for it).
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 am okay with 1e-10. I think we only document pi/2 as the limit and here we are just trying to avoid being too strict where in practice the user won't notice the difference.
There was also the point about the asymmetry with negative angles. I am not sure what to do about that right now. The backend will error right now if it gets a negative number. We could change the backend to round small negative numbers to 0 if we wanted symmetry of behavior for the bounds.
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.
Based on this conversation, I'm keeping the current behavior for the negative angles. Let me know if it's a problem.
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.
All conversations must be resolved before merging. Can we mark this as resolved @ihincks? I think the remaining options would be to round up small negative angles/parameters to 0 (a change of scope since the validation code only checks without modifying right now) or to change the backend code to accept small negative angles (out of scope here for now).
Co-authored-by: Ian Hincks <[email protected]>
Co-authored-by: Ian Hincks <[email protected]>
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.
This looks good to me except for a minor formatting change on the release note. I put in two other minor suggestions.
Co-authored-by: Will Shanks <[email protected]>
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.
Looks good to me!
Summary
Fixes #2000.
This is only a draft. I forgot to write the tests before the code. So the current push contains the logic, but the tests will be added shortly.
Details and comments
@SamFerracin I have a question about the noise learner. I see that
NoiseLearner._run
callsvalidate_isa_circuits
, likeBasePrimitiveV2
. I added inBasePrimitivesV2
a call to a new function,validate_rzz_pubs
. But I'm not familiar enough with the noise learner to figure if a similar call should be added there too. I tried to add, butNoiseLearner._run
receives circuits, unlikeBasePrimitiveV2
, which receives pubs. Should I make_run
receive pubs and add the call to the new function?