-
Notifications
You must be signed in to change notification settings - Fork 317
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
Deprecate convenience methods on Instrument classes #6086
Conversation
dcaedae
to
de2e9c2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6086 +/- ##
==========================================
- Coverage 67.35% 67.34% -0.01%
==========================================
Files 352 352
Lines 32139 32143 +4
==========================================
+ Hits 21646 21647 +1
- Misses 10493 10496 +3 ☔ View full report in Codecov by Sentry. |
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 support this deprecation. The only usefulness related to this that I can recall is calling parameters/methods/functions on channel lists, but that I already implemented and work for both proper parameters and bound methods (right?)
Yes, there was a limitation where you could not call a method on more than one channel (which was an advantage of a qcodes function) but that has since been lifted. |
4cadd07
to
67ebf22
Compare
67ebf22
to
a8e8f97
Compare
The aim is to reduce the number of ways in which the same operation can be performed
a8e8f97
to
1822a10
Compare
This deprecation results in quite some warnings in our code. I agree that the code is cleaner, but I don't like it that code used in the labs will break in many places when this functionality is removed. |
@sldesnoo-Delft Thanks for your input. In the code that I had access to I saw very few uses of these patterns so good to know they are being used elsewhere. I would suggest that we leave this code deprecated but there is no reason that we have to remove it any time soon if there is real usage of it. That way users will be guided in the right direction. BTW since we are using the new 3.13 deprecated decorator (backported from typing extensions) you can use a type checker for catching most usage of deprecated code. I have been using pyright with a config like this (pyright.json)
as a quick way to find all calls to deprecated methods etc. I believe that mypy is also in the process of adding support for this in the next release or so. |
I just want to chime in here as someone from a lab that has built a software package where many objects (qubits, AWGs, etc.) are instances of qcodes InstrumentBase. While we will refactor the several hundred instances of get/set, I would like to understand the reasoning here a bit better. I also briefly note that these methods are not shown as deprecated in the API documentation |
Following the discussion in #6080 I think we should consider getting rid of these methods on the instrument classes.
All of this functionality can be done in ways that is more obvious, direct and more frequently used
E.g. parameter and functions can be looked up from the
parameters
andfunctions
delegate attr dicts if you need to look them up dynamically.