Deprecation of Qdac in favor of QDac_channels #1578
Conversation
| @@ -30,7 +31,8 @@ class QDac(VisaInstrument): | |||
|
|
|||
| # set nonzero value (seconds) to accept older status when reading settings | |||
| max_status_age = 1 | |||
|
|
|||
| # this driver will be deprecated in favor of QDac_channels.py | |||
| @deprecate | |||
There was a problem hiding this comment.
allow me to suggest to have a dedicated deprecate decorator for __init__ - the warning message could then say that "some CLASS is deprecated, and other class should be used" instead of say "function SMTH is deprecated...", see here:
https://github.com/QCoDeS/Qcodes/blob/0f9176495d3bd02e6a05a221ef0b9635810c0f02/qcodes/utils/deprecate.py#L10-L10
| @@ -30,7 +31,8 @@ class QDac(VisaInstrument): | |||
|
|
|||
| # set nonzero value (seconds) to accept older status when reading settings | |||
| max_status_age = 1 | |||
|
|
|||
| # this driver will be deprecated in favor of QDac_channels.py | |||
| @deprecate | |||
There was a problem hiding this comment.
please use reason and alternative arguments of this decorator to provide our users with reason and alternative. after that, the comment this driver will be deprecated in favor of QDac_channels.py will be redundant. (Also note that it is extremely unlikely that the users will read that comment.)
Codecov Report
@@ Coverage Diff @@
## master #1578 +/- ##
==========================================
- Coverage 67.23% 67.17% -0.06%
==========================================
Files 145 145
Lines 17938 17939 +1
==========================================
- Hits 12060 12050 -10
- Misses 5878 5889 +11 |
| def __init__(self, name, address, num_chans=48, update_currents=True): | ||
| warnings.warn("The QDac Class is deprecated; use QDacChannel instead.", DeprecationWarning, 2) |
There was a problem hiding this comment.
I don't mean to nitpick, but the QDac class from QDac.py is being deprecated in favour of the QDac class from QDac_channels.py. The module QDac_channels.py also contains a class called QDacChannel, but that is not an instrument driver; the driver is called QDac.
There was a problem hiding this comment.
Why are we no longer using the decorator. Also as @astafan8 pointed out somewhere deprecation warnings are hidden by default
|
@jenshnielsen The deprecation decorator is implemented in such a way that it raises a warning for function deprecations. In this case the whole module is deprecated. @WilliamHPNielsen @astafan8 Moreover, DeprecationWarning is hidden by default if one does not specify the stacklevel (or trigger the warning in __ main __ ). In this case stacklevel is specified and shows up correctly at the import time. |
|
@GateBuilder Note that it looks like there are differences between when DeprecationWarnings are shown in python 3.6 and 3.7 Compare https://docs.python.org/3.6/library/warnings.html#warning-categories to https://docs.python.org/3.7/library/warnings.html#warning-categories I suggest that we extend the decorator to also support classes. This could simply be done by taking an optional type argument. @astafan8 I suggest that we create a qcodes deprecation warning that inherit from UserWarning or some other warnings categorie that is not DeprecationWarning. Similar to how it is done here https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/cbook/deprecation.py in matplotlib |
|
@GateBuilder I'm not sure I understand why the reality is such that we should remember to "specify the stacklevel" to some value when issuing a warning. @jenshnielsen I like your suggestion to create I am also for extending the |
We already depend on this
|
I think this looks good. I suggest we merge this via a squash merge as the history has become somewhat confusing on this branch. |
|
Sorry for the commit mess, I forgot that I did not have a clean master... I added reverting commits and will squash merge |
|
I always try to remember merging and rebaseing against origin/master and not the local master to prevent this |
|
I pulled from upstream, but I think the problem was that I created the branch via |
Changes proposed in this pull request:
Added deprecation call for QDac in favor of QDac_channels to prevent confusion.
@Dominik-Vogel