-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Concise StandardGate::<ID>
enums in Rust space
#14026
Conversation
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 13896854086Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - 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.
This seems like a nice improvement to me. Just a few minor comments.
(I didn't look too closely for typos--I assume you used an IDE to do symbolic renaming 😉)
StandardGate::Z, | ||
StandardGate::T, | ||
StandardGate::S, | ||
StandardGate::Tdg, |
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.
Is there a good reason it's Tdg
and not TDG
, or is that just historical?
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's a good historical reason 😛 See #3695 and the associated issue, we decided the naming norm is: a single uppercase letter per gate type (e.g. T
for T-gate, C
for controlled, R
for a rotation, S
for square root...) and dg
being just an inverse qualifier remains lowercase. Ofc there are exceptions, like SwapGate
(no single letter gate type), or SGate
(which I guess would technically be SZ
as square root of Z) but that's minimal 🙂
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.
I think it's important for consistent naming in our C-API, and @mtreinish could have arbitrarily chosen to do this in the original standard gates PR, so I feel confident to merge this without more consensus. We can always change this again if anyone comes up with a good reason.
* Rename `StandardGate::([A-Z])*Gate --> StandardGate::$1` * patch comments * patch more comments * `cargo fmt` * fix rust class ref & full qualifiers in CommCancel
Summary
The Rust-side, internal
StandardGate
enum variants all repeat the fact that they are a gate. For example, the Hadamard isStandardGate::HGate
instead of simplyStandardGate::H
. Being an enum variant, the fact that it is a gate is evident and always attached to the object. I suspect this is mainly a legacy naming from moving over from Python space (or maybe @mtreinish comes remembers 🙂)?I'd suggest to shorten the enum variants to just the gate ID. This makes the code more concise and avoid redundant qualifiers. It also enables that gate enums in the C API, introduced in #14006, can be called
QkGate_H
instead ofQkGate_HGate
.