-
Notifications
You must be signed in to change notification settings - Fork 15
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
Rename Qblox module classes #766
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #766 +/- ##
=======================================
Coverage 62.45% 62.45%
=======================================
Files 47 47
Lines 5867 5867
=======================================
Hits 3664 3664
Misses 2203 2203
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -18,7 +18,7 @@ | |||
from .sweeper import QbloxSweeper, QbloxSweeperType | |||
|
|||
|
|||
class ClusterQRM_RF(ClusterModule): | |||
class QrmRf(ClusterModule): |
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 @PiergiorgioButtarini. I partly agree with the renames, particularly the removal of _
. Maybe QrmRF
would look better but no strong opinion from my side. Maybe we could also ask @aorgazf.
Other than that, could you please update the platforms in the other repo so that we can test?
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.
Yes I also prefer QrmRF
but I wanted to follow @alecandido suggestions. I think also Alvaro will prefer this notation.
Sure I will open a PR with the updated runcards
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.
As I said, there is some arbitrariness involved on which acronym you want to turn into words. You could consider a single acronym as a whole, and just use Qrmrf
(to me, it's the ugliest).
Within my personal arbitrariness, I'd pick the current one. But the one suggested by you is also fine.
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 opened the relative PR qiboteam/qibolab_platforms_qrc#107.
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. I believe the instruments are not available for testing, but given that it is just a rename, it should be easy to identify any bugs so could be fine to even merge.
Regarding the name, I just suggested capital RF because it is closer to literature, does not strongly violate the naming convention (for that I am not sure though) and it is also used in a couple of places, such as
class RFSoC(Controller): |
and in #661. But generally, excluding extremely bad choices (eg.
class cluster_qcm_bb
), I don't have strong opinion because no matter how intuitive we try to make names, most users will still need to look at the docs/code to know what to import.
Indeed, as I said above, acronyms involve a certain degree of arbitrariness
For sure. I would avoid even the current one, because still wrong (because of the |
Instruments are now working and QPU tests are passing, so we can probably merge this if the names are final. |
I agree with @stavros11, this could be merged. It's a relatively small (and primarily internal) change, that has been already propagated consistently. |
Qblox modules now have conventional names. Also I removed a deprecated test file.
Closes #742
Checklist: