Feature/parameter node, change parameter get/set behaviour

[nulinspiratie]

This PR proposes to add the ParameterNode, as raised by @AdriaanRol in #773. My main goal was to have a different way of setting/getting parameters instead of using brackets (raised in #767). I've added a tutorial on how to use the ParameterNode in the PR.

Changes proposed in this pull request:

• Add ParameterNode
• Change get/set behaviour of parameters inside a node
  The proposed new method is:
  • Set: parameter_node.parameter = 42
  • Get: parameter_node.parameter (returns 42)
    These commands set/get the value of the parameter, and the underlying parameter is accessed differently (see below for details).
    Note that the parameter get/set itself isn't changed, if you have a parameter object, you should still use brackets.
• Replace InstrumentBase with ParameterNode
• Add ParameterNode tutorial

I think the main advantage of the new get/set method is that one doesn't need to think if an attribute is actually a Parameter or not. This allows you to replace attributes with parameters as you see fit, while also being able to keep other attributes the way they are. Having parameters behave as attributes also feels more pythonic IMHO.

One potential issue with this new method is that any time a parameter attribute is accessed, this will cause a .get(), therefore, hasattr(parameter_node, 'parameter') will call its .get method. I'm not sure if this is an issue, as there aren't many cases where this happens.

Smaller changes

• Replace print_readable_snapshot with print_snapshot, the snapshot being readable should be a given.
• add dir of DotDict to include its keys
• Make name and Instrument of BaseParameter optional

I haven't implemented everything discussed in #773, I first wanted to get some thoughts on the new proposed method

Below is a summary of the ParameterNode, this is explained in more detail in the added tutorial notebook

Creating a ParameterNode

Parameter nodes are instantiated via: parameter_node = ParameterNode()

Adding parameters

parameters can be added by setting an attribute:
parameter_node.new_parameter = Parameter()
Note that it is no longer necessary to specify a name in the Parameter. If it is not explicitly specified, it will be set to the attribute name

Getting/setting parameter values

Getting and setting of parameters in a parameter node is now done same as you would any other attribute:

• Setting a value: parameter_node.new_parameter = 42
• Getting a value: parameter_node.new_parameter (returns 42)

Accessing parameter object

The actual parameter can be accessed in two ways:

1. parameter_node.parameters.new_parameter (parameter_node.parameters is a DotDict).
2. parameter_node().new_parameter, the call () will return parameter_node.parameters.

The second method is more succinct, but does have one drawback, namely that autocomplete won't work with it. I haven't figured out another way of easily accessing parameters that does have autocomplete, or a way to add autocompletion to the call method.

Nesting nodes

Parameter nodes can also be nested, similarly to how parameters are added:
parameter_node.nested_parameter_node = ParameterNode().
This sets the nested parameter node's name to the attribute.

@AdriaanRol @jenshnielsen @WilliamHPNielsen @giulioungaretti @alexcjohnson @alan-geller @spauka

[WilliamHPNielsen]

@nulinspiratie, on behalf of the Copenhagen devs:

We certainly appreciate your effort, but need to raise a few words of caution.

Even without having looked at the code in any detail, learning from our experience with our last big change to the parameters, where we had to clean up and hotfix for months after merging, there is no way such a fundamentally changing PR can be merged in one go.

Changing the API of Instrument is like a major version revision, something that we can't really bump without a long and careful discussion. Perhaps we even need a PEP/Spec-like system for these big changes (we could call them QEP's?). AS you are of course well aware, there has already been some discussion about parameter nodes, but it came to a halt before converging.

The above entails that the new Instrument would only be merged into QCoDeS via a fully independent alternative to the current Instrument that does not touch the existing Instrument and Channel classes.

The work flow would be something like:

• Add parameter node
• Port existing drivers in qcodes over slowly one by one. This will take month if not years.
• Then deprecate the existing Instrument/Channel classes.
• And perhaps eventually remove it fully. That would be long time into the future.

This may seem like an unfulfilling snail pace to you, but we have a large user groups whose day-to-day work depends on QCoDeS.

[nulinspiratie]

@WilliamHPNielsen Thanks for your reply. I definitely agree that this would be a significant change, and would break compatibility, which is a big issue affecting many users. I wasn't expecting this to be merged any time soon, if at all. However, It's something our fork will probably switch to, and I thought I'd place it here as a potential future direction for parameters (QEP does sound nice!).

[AdriaanRol]

Hi Serwan, cool that you guys are still innovating on your QCoDeS fork!

I personally do not like the change in the get/set behaviour. I think there were good reasons why it is implemented the way it is and to be honest I personally think the way that part works is really solid and should not be changed.

Another item (as mentioned by @WilliamHPNielsen ) is the potentially breaking backwards compatibility . I am paranoid about those and for me that is the chief reason that sometimes delays updates and creates diverging branches/forks, something which should be avoided in my opinion.

I do think a first implementation of the parameter node is cool and potentially very useful, though I do not understand why you changed the inheritance for the base instrument. I also like your example notebook but I would like to have some more detail in there, mostly to see how it support the print_readable_snapshot (without the name changed) and what regular snapshots look like.

I am afraid that in it's current form the node-tree functionality get's buried in the alternative get/set behaviour. I do think it is great as a conversation starter though :).

[nulinspiratie]

Hey @AdriaanRol thanks for the useful comments. I definitely see your point that the get/set behaviour works well for Instruments, especially because it makes explicit when VISA commands are communicated with the physical instrument. I've made some changes to the ParameterNode that keeps this behaviour and may be a good middle way (see below). The reason why I removed the instrument inheritance from InstrumentBase to ParameterNode is that the InstrumentBase already functioned as a pretty good ParameterNode, and so I simply added a few functionalities and replaced the name. I think ParameterNode is better suited, since it can be used for things completely unrelated to instruments.

@WilliamHPNielsen Breaking backwards compatibility is definitely an issue, and I've been thinking how to deal with it. I think I've found a solution that works, namely by adding the flag use_as_attributes during the ParameterNode initialization. In short, it works as follows:

Setting use_as_attributes=False, which is the default

parameter_node = ParameterNode() # use_as_attributes is False by default
parameter_node.new_parameter = Parameter(set_cmd=None)
parameter_node.new_parameter = 42 # Set parameter value works as above
parameter_node.new_parameter(42) # Set parameter value can also be done via brackets like before
parameter_node.new_parameter # Returns Parameter instance.
parameter_node.new_parameter() # Returns Parameter value

This is the default behaviour of the ParameterNode, and pretty much behaves the same way as before. Accessing the Parameter attribute of a ParameterNode returns the Parameter, and not its value.
In this case there are three differences compared to before, which all shouldn't break compatibility:

1. Setting a parameter value can also be done via parameter_node.new_parameter = 42. Currently we never set a parameter attribute, and so shouldn't break anything. We can also remove this behaviour if its unwanted.
2. The Parameter name doesn't need to be specified. If unspecified, it will set its name to that of the attribute.
3. Parameters can be added to the ParameterNode (and hence also Instruments) by simply setting its attribute. I also kept add_parameter to retain compatibility.

Pretty much all the tests still work, and I don't see why this wouldn't be backwards compatible. Would this be a viable solution?

Setting use_as_attributes=True

parameter_node = ParameterNode(use_as_attributes=True)
parameter_node.new_parameter = Parameter(set_cmd=None)
parameter_node.new_parameter = 42 # Set parameter value
parameter_node.new_parameter # Returns 42

In this case, the Parameter attributes in the ParameterNode behave like the values they represent. This is best suited for more abstract classes (meta-instruments) that still need parameter functionality. An example is a Pulse object, whose parameter Pulse.duration can be varied in a loop, which in turn will program the instruments to output the pulse for a specific duration. In this case, it is more convenient to treat the parameter as an attribute, as the Pulse may have many more attributes, some of which aren't parameters (e.g. @property attributes). If getting/setting parameters happens different than other attributes, you would have to remember each time if an attribute is actually a parameter or not. For our group, the ability to access parameters as attributes definitely improves things.

[codecov]

Codecov Report

Merging #935 into master will increase coverage by 9.93%.
The diff coverage is 68.58%.

@@            Coverage Diff             @@
##           master     #935      +/-   ##
==========================================
+ Coverage   67.21%   77.14%   +9.93%     
==========================================
  Files         145       38     -107     
  Lines       17951     5289   -12662     
==========================================
- Hits        12065     4080    -7985     
+ Misses       5886     1209    -4677

[nulinspiratie]

@WilliamHPNielsen @jenshnielsen Do you think this PR has a shot at being accepted? I understand the hesitancy to change core functionalities, but I do think the change wouldn't cause breaking changes. I've purposely left the old usage intact, and added functionality on top of it. This means that all the drivers etc. should still work as is. The tests are basically passing (one sometimes fails, but seems unrelated to this PR), and I think it would add an avenue to integrate Parameters more easily into classes that are unrelated to Instruments. If not, then we can close this PR.