Qcodes icon indicating copy to clipboard operation
Qcodes copied to clipboard

Deprecate convenience methods on Instrument classes

Open jenshnielsen opened this issue 1 year ago • 2 comments

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 and functions delegate attr dicts if you need to look them up dynamically.

  • [x] Fix any use of this in qcodes
  • [ ] Should check agains contrib drivers and perhaps zhinst-qcodes and others
  • [ ] Newsfragment if we want to merge this

jenshnielsen avatar May 17 '24 14:05 jenshnielsen

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.34%. Comparing base (badc122) to head (f02bdc2). Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
src/qcodes/instrument_drivers/tektronix/AWG5014.py 50.00% 2 Missing :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 17 '24 20:05 codecov[bot]

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.

jenshnielsen avatar May 21 '24 12:05 jenshnielsen

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 avatar Oct 18 '24 09:10 sldesnoo-Delft

@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)

{
    "typeCheckingMode": "off",
    "reportDeprecated": true
}

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.

jenshnielsen avatar Oct 23 '24 13:10 jenshnielsen

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. This change results in a lot of deprecation warnings at once. Many measurements may now emit more than 10 of these.

While we will refactor the several hundred instances of get/set, I would like to understand the reasoning here a bit better. Are there any performance or other technical reasons for this change, or is it rather one of code organization? #6080 doesn't seem to explicitly discuss get and set, but rather focuses on indexing.

I also briefly note that these methods are not shown as deprecated in the API documentation

grahamnorris avatar Nov 21 '24 08:11 grahamnorris

@grahamnorris

Thanks for your message #6657 will add the deprecation to the docstrings.

We made this deprecation since these methods do not add any value to qcodes. The main aim is to make it easier for users to

Some of the issues

  • Looking up a parameter by name is not type safe unlike using the attribute. As you cannot know the type of instrument["myparam"]
  • They are surprising and not very pytonic. E.g. In python you don't look up attributes by name on classes using __getitem__/[] That is something that is reserved for dicts/mappings. You dont call an object by passing the name of that object to a function etc.
  • They violate the zen of python. There should be one-- and preferably only one --obvious way to do it. Since they add an additional way to do things and they are not obvious.

Note that as with all other python warnings you have the option to silence them until you have the time to perform a transition.

https://docs.python.org/3/library/warnings.html#the-warnings-filter

jenshnielsen avatar Nov 22 '24 10:11 jenshnielsen