Add paramterScaler with an usage example and deprecate VoltageDivider #1069

YakBizzarro · 2018-04-23T17:10:21Z

Here I propose an enhanced version of the VoltageDivider. Compared to it, it has the ability to set either a division or a gain, depending on the nature of the instrument attached to it. Unit of the resulting parameter can be changed. Moreover, the gain/division can be a Qcodes parameter, in the case that is set by a remote instrument and not locally.
We have been using this object in SpinQubit group in QuTech for a few months with modules in IVVI/SPI racks, so it's well tested on the field.

@WilliamHPNielsen, @jenshnielsen

codecov · 2018-04-23T17:39:56Z

Codecov Report

Merging #1069 into master will increase coverage by 0.2%.
The diff coverage is 94.04%.

@@            Coverage Diff            @@
##           master    #1069     +/-   ##
=========================================
+ Coverage   79.96%   80.16%   +0.2%     
=========================================
  Files          49       49             
  Lines        6673     6762     +89     
=========================================
+ Hits         5336     5421     +85     
- Misses       1337     1341      +4

YakBizzarro · 2018-05-25T08:11:48Z

Any news for this PR? @WilliamHPNielsen, @jenshnielsen

jenshnielsen · 2018-06-04T06:41:50Z

Sorry for the delay @YakBizzarro I am not absolutely sure this it the way we want to do this in the long term but for now it should be fine. I would prefer not to deprecate the voltage divider just yet. And I would very much like to see this have some tests if possible

YakBizzarro · 2018-06-04T09:48:25Z

Thank you for the feedback @jenshnielsen , I'll try to write some tests. BTW, what are the plans for the long term?

jenshnielsen · 2018-06-04T13:32:30Z

Not very concrete at the moment but we want to design some more generic objects that can abstract multiple instruments and how they interact

YakBizzarro · 2018-07-02T17:44:27Z

@jenshnielsen tests added

astafan8

Thank you for implementing this! But please fix some software-design related things: the cleaner the implementation, the more stable and useful the feature.

astafan8 · 2018-07-03T07:27:46Z

qcodes/instrument_drivers/devices.py

+        is_divider = division is not None
+        is_amplifier = gain is not None
+
+        if not (is_divider ^ is_amplifier):


For readability reasons, coups you use ‘xor’ from ‘from operator import xor’? :)

astafan8 · 2018-07-03T07:28:20Z

qcodes/instrument_drivers/devices.py

+    DIVISION = enum.auto()
+
+
+class ParameterScaler(Parameter):


Great addition to qcodes! Thank you!

astafan8 · 2018-07-03T07:33:17Z

qcodes/instrument_drivers/devices.py

+            instrument=getattr(self._wrapper_param, "_instrument", None),
+            label=self.label,
+            unit=self.unit,
+            metadata=self._wrapper_param.metadata)


What would be the result of implementing metadata inclusion like this? Are the attributes of the ‘_wrapper_parameter’ going to be included into the snapshot next to the attributes of the scaler, or in a separate field? I prefer the latter case.

I removed the metadata inclusion from the parent. As long as the parent instrument is noted in the metadata, there is no need to copy its metadata

astafan8 · 2018-07-03T07:35:04Z

qcodes/instrument_drivers/devices.py

+                 name: str=None,
+                 label: str=None,
+                 unit: str=None) -> None:
+        self._wrapper_param = output


Could you please rename ‘_wrapper_parameter’ to ‘_wrapped_parameter’? The reason is that scaler wraps the output, hence the output is wrapped inside the scaler, and the scaler is the wrapper here.

astafan8 · 2018-07-03T07:38:09Z

qcodes/instrument_drivers/devices.py

+            self.label = name
+        else:
+            if self.role == Role.DIVISION:
+                self.label = "{}_attenuated".format(self._wrapper_param.label)


The amount of times decisions are made based on the ‘role’ means that this should be implemented via factory so that the code is more maintainable. Is it clear what I am suggesting, or do you need help/support with this?

Yes, I can't really understand this. Do you have some example/doc?

:) There is a design pattern called "factory", it allows you to create similar objects while keeping the specifics of the objects inside them. For example, now the ParameterScaler can be a divider, an amplifier, and also knows how to choose its role. The class becomes too big, complex, with repetitive code (the if role == .. stuff), not easily testable as you have to remember about all the possible things the class can do. Instead, an more maintainable design is to have a Divider class, an Amplifier class, and a "factory" class that provides the interface for creating them.

You can read more on the wiki or take a look at a somewhat close example in Python.

So my proposal would be to:

define Divider class (perhaps private so that it cannot be directly instantiated) that only divides

define Amplifier class (perhaps private so that it cannot be directly instantiated) that only amplifies

keep the ParameterScaler class as an "interface" that users will use to create Dividers or Amplifiers based on the key-value argument (gain= or division=)

refactor existing tests so that they correspond to these classes.

@YakBizzarro , in case you don't have time or you find it troublesome to implement the factory, would you allow me to do it? (this will mean that I will push some commits to you branch)

astafan8 · 2018-07-03T07:41:39Z

qcodes/instrument_drivers/devices.py

+
+    @gain.setter
+    def gain(self, gain: Union[int, float, Parameter]):
+        self.role = Role.GAIN


Do you see any use in changing the role after the ScalerParameter object is created? I personally don’t because it may lead to errors in users code (also when the role is changed, labels and names are not updated) - thus, I’d suggest that if the role is “gain”, it should not be possible to set the “division” and vice versa.

On the contrary, I found very useful sometimes when changing the gain, to change also the way how's expressed (division or gain). Also some instruments have a variable gain that can be either less or more then one. I would rather remove the _attenuated/_amplified naming and use only "_scaled"

astafan8 · 2018-07-03T07:45:19Z

qcodes/instrument_drivers/devices.py

+        self._save_val(value)
+        return value
+
+    def get_instrument_value(self) -> Union[int, float]:


Why is it called instrument value? What is returned is the value of the wrapped parameter, so I expect that the name is smth like “get_unscaled_value” or better “get_wrapped_parameter_value”.

astafan8 · 2018-07-03T07:46:05Z

qcodes/instrument_drivers/devices.py

+
+    def set_raw(self, value: Union[int, float]) -> None:
+        """
+        Se the value on the instrument, accounting for the scaling


Typo : “set”

Typo: not “instrument” but “wrapped parameter” or “parameter that is being scaled”

astafan8 · 2018-07-03T07:51:24Z

qcodes/instrument_drivers/devices.py

+            instrument_value = value * self._multiplier()
+
+        # disable type check due to https://github.com/python/mypy/issues/2128
+        instrument_value = cast(Union[int, float], instrument_value)


What is this needed for? Also, instead of union(int,float) could we use the “type” information from the wrapped parameter?

Just realized that the comment is wrong. This line was inserted in 43a4dd0 by @jenshnielsen , do you have any comment? I'm not very used to python typing system

astafan8 · 2018-07-03T07:53:59Z

qcodes/tests/test_parameter.py

@@ -904,6 +905,57 @@ def tearDown(self):
        del self.d


+class TestParameterScaler(TestCase):
+    def setUp(self):


Could you add tests for name, label, and unit logic? Also test for metadata working correctly? Also for the fact that an exception is thrown if one is creating a scaler parameter and supplies both gain and decision to the constructor?

…ture

YakBizzarro · 2018-07-04T13:32:40Z

@astafan8 Ok, I made the modifications that you suggested, except for the factory and role

YakBizzarro · 2018-07-04T13:39:30Z

Btw, initially I put this class in qcodes/instruments_drivers/devices.py because I was extending VoltageDivider, but since now it's a very generic helper, would it better to have it in qcodes/instrument/parameter.py?

astafan8 · 2018-07-04T14:25:09Z

indeed @YakBizzarro , very good remark, let's put this class into qcodes/instrument/parameter.py

spxtr · 2018-07-04T18:01:53Z

docs/examples/Paramter Scaler.ipynb

@@ -0,0 +1,252 @@
+{


docs/examples/Paramter Scaler.ipynb -> docs/examples/Parameter Scaler.ipynb

spxtr · 2018-07-04T18:09:18Z

qcodes/instrument_drivers/devices.py

+        is_amplifier = gain is not None
+
+        if not xor(is_divider, is_amplifier):
+            raise ValueError('Provide only division OR gain')


Why not allow both? Have both default to 1 instead of None, drop self.Role, in the constructor set self._multiplier = gain / division, and have get_raw return self.wrapped_parameter() * self._multiplier()? I'm probably missing something, but that seems simpler in both usage and implementation.

I don't see an use case for having both a multiplier and a divider. Currently they are just defined as the mutual reciprocal (divider = 1/gain or gain = 1/divider) as user convenience. For example, for a a resistive divider, you can say that is divided by 1000 or that it has a gain of 0.001, and it's totally equivalent

I guess the root of my confusion is that it doesn't seem like the Role enum is necessary at all. If they call division then just divide_multiplier by the given factor.

You have this:

class Role(enum.Enum): GAIN = enum.auto() DIVISION = enum.auto() def __init__(self, output: Parameter, division: Union[int, float, Parameter] = None, gain: Union[int, float, Parameter] = None, name: str=None, label: str=None, unit: str=None) -> None: # (...) # Set the role, either as divider or amplifier # Raise an error if nothing is specified is_divider = division is not None is_amplifier = gain is not None if not xor(is_divider, is_amplifier): raise ValueError('Provide only division OR gain') if is_divider: self.role = ParameterScaler.Role.DIVISION self._multiplier = division elif is_amplifier: self.role = ParameterScaler.Role.GAIN self._multiplier = gain # (...) # Division of the scaler @property def division(self): if self.role == ParameterScaler.Role.DIVISION: return self._multiplier() elif self.role == ParameterScaler.Role.GAIN: return 1 / self._multiplier() @division.setter def division(self, division: Union[int, float, Parameter]): self.role = ParameterScaler.Role.DIVISION self._multiplier = division # Getter and setter for the real value def get_raw(self) -> Union[int, float]: """ Returns: number: value at which was set at the sample """ if self.role == ParameterScaler.Role.GAIN: value = self._wrapped_parameter() * self._multiplier() elif self.role == ParameterScaler.Role.DIVISION: value = self._wrapped_parameter() / self._multiplier() self._save_val(value) return value

Could you do this instead? To me at least it is much simpler.

def __init__(self, output: Parameter, division: Union[int, float, Parameter] = 1, gain: Union[int, float, Parameter] = 1, name: str=None, label: str=None, unit: str=None) -> None: # (...) self._multiplier = gain / division # (...) # Division of the scaler @property def division(self): return 1/self._multiplier() @division.setter def division(self, division: Union[int, float, Parameter]): self._multiplier = 1/division # Getter and setter for the real value def get_raw(self) -> Union[int, float]: """ Returns: number: value at which was set at the sample """ value = self._wrapped_parameter() * self._multiplier() self._save_val(value) return value

Not a big deal though, feel free to ignore :)

I like the idea of @spxtr that the Role is not needed, but the self._multiplier = gain / division part seems confusing. I'd suggest that one can initialize a ParameterScaler via gain OR division kwarg (as it is now), but instead of having a Role, either gain or division are converted to multiplier. I mean smth like this:

def __init__(self, output: Parameter, division: Union[int, float, Parameter] = 1, gain: Union[int, float, Parameter] = 1, name: str=None, label: str=None, unit: str=None) -> None: # (...) if not xor(gain is None, division is None): raise KeyError('Either gain or division should be supplied') if gain: # gain is defined as scaled_value/wrapped_value self.gain = gain # magic happens in the property setter if division: # division is defined as 1/(scaled_value/wrapped_value) self.division = division # magic happens in the property setter # (...) # Division of the scaler @property def division(self): return 1/self._multiplier() @division.setter def division(self, division: Union[int, float, Parameter]): self._multiplier.set(1/division) # Gain of the scaler @property def gain(self): return self._multiplier() @gain.setter def gain(self, gain: Union[int, float, Parameter]): self._multiplier.set(gain) # Getter for the real value def get_raw(self) -> Union[int, float]: """ Returns: number: value at which was set at the sample """ value = self._wrapped_parameter() * self._multiplier() self._save_val(value) return value

This eliminates the need for the role (testing is easier, no "factory" requests from me :) ).

@YakBizzarro what do you think?

Role is needed in the case when the multiplier is not a simple scalar, but a Qcodes parameter (look at the last example). In this case 1/division should be computed every time that we set a value, or (like a do), perform a multiplication, depending on the role value

Now I get it: if the multiplier is a qcodes parameter, then the ScaledParameter needs to know what the role of the multiplier is in order to perform the right operation (multiplication or division)...

ok, the number of responsibilities/blows/whistles of the ScaledParameter is exploding.

@YakBizzarro Could you list the use cases that this class is intended to support? Is the following list complete ? :

create a new parameter that has a fixed and unchangeable scaling factor (both <1 or >1) for another parameter

create a new parameter that multiplies another parameter with a third parameter

create a new parameter that divides another parameter by a third parameter
If so, then I see at least 3 different classes (and possibly a couple of extra private ones to "modularize" the implementation).

Right. Just in the first case, the scaling factor can be changed manually by the user using the setter of division/gain. Useful for IVVI modules with manual gain/division

The latter cases are intended for instruments where the gain is selectable by the computer or when is function of an external value

astafan8 · 2018-07-06T09:26:04Z

qcodes/instrument/parameter.py

@@ -1545,3 +1547,197 @@ def __init__(self, name, instrument=None, initial_value=None, **kwargs):
        super().__init__(name=name, instrument=instrument,
                         get_cmd=None, set_cmd=None,
                         initial_value=initial_value, **kwargs)
+
+
+class ParameterScaler(Parameter):


@YakBizzarro , sorry for the late comment, but I would like to rename this class to ScaledParameter. The reason is the following. A "scaler" (so far as I my knowledge of English is concerned) is smth that "scales" smth. According to the implementation, the internal "multiplier" parameter is doing the actual "scaling", while the "ParameterScaler" instance just returns the "scaled" value of the wrapped parameter. In other words, if I call get for the "ParameterScaler" I get the "scaled" value. Thus, "ParameterScaler" shall be called "ScaledParameter".

Ok, seems legit

astafan8 · 2018-07-06T09:28:47Z

qcodes/instrument/parameter.py

+        self._meta_attrs.extend(["role"])
+        self.metadata['wrapped_parameter'] = self._wrapped_parameter.name
+        try:
+            self.metadata['wrapped_instrument'] = self._wrapped_instrument.name


instead of try-catch please use getattr function with an "if" statement:

if self._wrapped_instrument: self.metadata['wrapped_instrument'] = getattr(self._wrapped_instrument, "name", None)

YakBizzarro · 2018-07-06T14:14:48Z

Is it good now or should I do some other modification?

astafan8 · 2018-07-06T14:19:44Z

@YakBizzarro yes, there is a discussion going on here about whether the "role" part is needed at all. could you take a look?

YakBizzarro · 2018-07-06T14:24:17Z

@astafan8 ops, completely missed it, thank you

astafan8 · 2018-07-26T12:51:56Z

@YakBizzarro After some time thinking about it and also discussing with the core team, my conclusion is that I am going to merge this PR "as is". I appreciate your patience, and effort spent in fixing comments and adding tests!

astafan8 · 2018-07-26T13:14:32Z

@YakBizzarro Please fix the mypy errors (see Travis output) and codacy complaints. And then I'll merge.

…into paramterscaler

YakBizzarro · 2018-07-30T07:24:50Z

@astafan8 Thank you! I fixed the mypy errors; most of the codacy issues are false positives (it complains that methods are not callable, which is true, but the values returned by this methods are callable), but I don't get why it complain about overloading get_raw/set_raw

astafan8 · 2018-07-30T08:47:01Z

@YakBizzarro Well, they are not false positives :)

the get_raw/set_raw one comes from a confusion with Parameter and _BaseParameter classes - in the first, you have to redefine get/set methods, while in the second you have to redefine get_raw/set_raw; but because constructor of Parameter calls the constructor of the _BaseParameter before dealing with get/set methods, you can redefine get_raw/set_raw even though you are inheriting from Parameter. But this is not your fault, we will clean up Parameter and _BaseParameter classes in some time in the future.

And the "_multiplier is not callable" can be solved by first getting it into a local variable like multiplier_param = self._multiplier, and then calling the new variable. Extra type annotations can help with making this code understandable.

But as discussed before, this implementation has quite some other more important problems, so for the sake of time, let's not bother about these minor issues now. I am going to merge.

Merge: e9e6f1f 723d55a Author: Mikhail Astafev <astafan8@gmail.com> Merge pull request #1069 from YakBizzarro/paramterscaler

YakBizzarro added 2 commits April 23, 2018 19:05

Add paramterScaler with an usage example and deprecate VoltageDivider

640ab51

Add missing import

d6151b2

Add tests for ParameterScaler

7c08099

Merge branch 'master' into paramterscaler

074d57e

astafan8 requested changes Jul 3, 2018

View reviewed changes

Improve structure of ParameterScaler and adapt tests to the new struc…

74baebb

…ture

spxtr reviewed Jul 4, 2018

View reviewed changes

YakBizzarro added 4 commits July 5, 2018 10:39

style: remove some useless whitespaces

dd0e5e6

Move ParameterScaler to instrument/parameter.py

3e19074

Improve metadata for wrappped parameter in ParameterScaler

0fa0a02

Remove deprecation warning from VoltageDivider

e872aa0

astafan8 added the enhancement label Jul 5, 2018

astafan8 requested changes Jul 6, 2018

View reviewed changes

YakBizzarro added 2 commits July 6, 2018 11:54

rename ParameterScaler to ScaledParameter

ff31c09

Improve code quality in ScaledParameter

875bdd4

astafan8 approved these changes Jul 26, 2018

View reviewed changes

Merge branch 'master' into paramterscaler

c863cd5

astafan8 assigned YakBizzarro Jul 26, 2018

YakBizzarro added 2 commits July 30, 2018 09:21

fix: remove uneeded type annotation in ScaledParameter

c28f9a8

Merge branch 'paramterscaler' of https://github.com/YakBizzarro/QCodes …

723d55a

…into paramterscaler

astafan8 merged commit 82cb20e into microsoft:master Jul 30, 2018

giulioungaretti pushed a commit that referenced this pull request Jul 30, 2018

Generated gh-pages for commit 82cb20e

92e6f8b

Merge: e9e6f1f 723d55a Author: Mikhail Astafev <astafan8@gmail.com> Merge pull request #1069 from YakBizzarro/paramterscaler

Add paramterScaler with an usage example and deprecate VoltageDivider #1069

Add paramterScaler with an usage example and deprecate VoltageDivider #1069

Conversation

YakBizzarro commented Apr 23, 2018 • edited by WilliamHPNielsen

codecov bot commented Apr 23, 2018 • edited

Codecov Report

YakBizzarro commented May 25, 2018

jenshnielsen commented Jun 4, 2018

YakBizzarro commented Jun 4, 2018

jenshnielsen commented Jun 4, 2018

YakBizzarro commented Jul 2, 2018

astafan8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YakBizzarro commented Jul 4, 2018

YakBizzarro commented Jul 4, 2018

astafan8 commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spxtr Jul 5, 2018 • edited

Choose a reason for hiding this comment

astafan8 Jul 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astafan8 Jul 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YakBizzarro commented Jul 6, 2018

astafan8 commented Jul 6, 2018

YakBizzarro commented Jul 6, 2018

astafan8 commented Jul 26, 2018 • edited

astafan8 commented Jul 26, 2018

YakBizzarro commented Jul 30, 2018

astafan8 commented Jul 30, 2018

YakBizzarro commented Apr 23, 2018 •

edited by WilliamHPNielsen

codecov bot commented Apr 23, 2018 •

edited

spxtr Jul 5, 2018 •

edited

astafan8 Jul 6, 2018 •

edited

astafan8 Jul 6, 2018 •

edited

astafan8 commented Jul 26, 2018 •

edited