microsoft / Qcodes

Modular data acquisition framework
http://microsoft.github.io/Qcodes/
MIT License
337 stars 315 forks source link

optional arguments to Functions #51

Closed alexcjohnson closed 8 years ago

alexcjohnson commented 8 years ago

This is something @AdriaanRol and I have talked about before, and I just noticed his comment in the AWG5014 driver: "set function has optional args and therefore does not work with QCodes." I still think that for Parameters there should always be one and only one argument, because a Parameter is supposed to be a single degree of freedom and other factors, like how you set it to that value, are not supposed to matter, just the value you set it to. I can certainly see wanting to do this, when writing drivers, but I think if we can maintain this restriction, it will pay off for both our users (in terms of conceptual simplicity of what a Parameter is and how to use it) and for our core code (which then only ever has to handle a single value when setting a Parameter)

However, a Function does not represent a single degree of freedom, it represents some operation applied to the instrument, and here optional arguments make a lot of sense as they do with any Python function. You can of course already do this by just attaching a method to the instrument, rather than using an explicit Function object via the add_function method, but the advantages of add_function are:

So with that in mind, how do we implement optional arguments in Function? Right now the arguments are specified as a list of Validator objects. This already drops what each argument means - ie it's positional only, no name. It would be great if we could name all the args, and accept them the same ways regular functions accept them, as positional or keyword, and with default values where appropriate.

On the call side I think it's clear what to do, but how do we specify these args when creating the Function? We could make args be a list of (Validator, name[, default]) where an arg is required if default is not provided (and of course required args must come before optional). Or we could be more explicit and make args a list of dicts {'validator': Validator, 'name': name[, 'default': default]}

(as a related note, I'm currently refactoring Parameters and Functions anyway per #42 and Function used to call these parameters but that's obviously confusing vs Parameter - so I changed them to args)

AdriaanRol commented 8 years ago

@alexcjohnson , I am not entirely convinced of the advantage of the add Function functionality

The advantages you list are as follows

  • you can use command strings, not just wrap other real functions

I don't know why you would want to use command strings (and the additional string formatting) when you can instead use arguments to a function call. I consider relying on string formatting dirty coding and it commonly results in hard to read code.

  • you get built-in parameter validation, and valid inputs are all collected in one place

Do you not also get this the moment any parameter is touched through a function?, Additionally it is pretty straightforward to include validators in the function definition.

  • the function is listed in self.functions in case a user wants to list all the instrument functionality, separate from the base instrument methods

Fair enough, but at this point the distinction is more easily made by using underscore for those base methods we do not want to have.

It would be great if we could name all the args, and accept them the same ways regular functions accept them, as positional or keyword, and with default values where appropriate.

This bit worries me, to me it would be a major drawback if you do not have the default python functionality. A function should be a function and not a Function. I do not see the advantage of reinventing the wheel here, especially if it disallows using most default functionality.

Another thing that worries me is that all this wrapping will disallow access to the docstrings from the notebook level, note that this is already the case for the default parameters, not to mention the introduction of another new concept that is very similar but slightly different from what you expect it to be.

alexcjohnson commented 8 years ago

I am not entirely convinced of the advantage of the add Function functionality

It's true, it's not nearly as useful as Parameter, and you raise good points about introducing complexity while reinventing the wheel

I don't know why you would want to use command strings (and the additional string formatting) when you can instead use arguments to a function call. I consider relying on string formatting dirty coding and it commonly results in hard to read code.

You're always eventually going to be formatting to a string, to send over the comm channel, right? Specifying a command string is only useful if it cuts out boilerplate and lets you go straight to that step.

you get built-in parameter validation, and valid inputs are all collected in one place

Do you not also get this the moment any parameter is touched through a function?, Additionally it is pretty straightforward to include validators in the function definition.

You get parameter names and defaults together, but not validation. It's not hard to add, no, but it's a fairly large amount of repetitious code that people tend to get lazy and omit (and I'd argue, if you need to scan past this to see what the function really does, it makes the function harder to read). Perhaps the solution is to just make a generic validate_all method that takes in perhaps a list of (validator, value) couplets, that people can put at the top of their function definition to get the benefit (telling people what the requirements are) without all the explicit testing and raising that makes it tedious to write and hard to read?

def set_pulse(self, low_value, high_value, low_time, high_time=None):
    # if defaults are not already valid values, handle them first
    if high_time is None:
        high_time = low_time
    # then check all the values at once - which also clearly documents what's valid
    self.validate_all(
        (vals.Numbers(-10, 10), low_value),
        (vals.Numbers(-10, 10), high_value),
        (vals.Ints(1, 1e6), low_time),
        (vals.Ints(1, 1e6), high_time))
    # then the actual actions
    self.write('set:pul: {:.4f} {:.4f} {} {}'.format(low_value, high_value, low_time, high_time))

... it would be a major drawback if you do not have the default python functionality. ... ... all this wrapping will disallow access to the docstrings from the notebook level ...

fair points. add_function was made to handle simple things like *RST without a lot of boilerplate, but if we try to make it into a full-fledged replacement for actual functions we'll just miss things and confuse people. OK, so I'll shelve the argument enhancements and encourage making real methods for all but the simplest functions.

AdriaanRol commented 8 years ago

You're always eventually going to be formatting to a string, to send over the comm channel, right?

Ah, now I better understand where the add-function comes from. I'd say this is not generally True, in the case of VISA instruments it is, however there is a whole range of different instruments that we want to use instrument drivers for.

To give some examples

validate_all method that takes in perhaps a list of (validator, value) couplets,

I like this :+1: , however maybe not make it part of the inheritance structure but rather something you can import from utils? I think the concept of using multiple validators should be applicable outside of just the Instrument class.

def set_pulse(self, low_value, high_value, low_time, high_time=None):
     ....

I see examples of sequencers and pulses coming up all the time, I am still a bit skeptical if we can find the right abstraction to make it indeed as simple as the example you sketch here. I would love it if it is but fact is that all the low level instruments all have their own peculiarities for these kind of things.

alexcjohnson commented 8 years ago

however maybe not make it part of the inheritance structure but rather something you can import from utils?

Good call - I'll just put it in validators.py

I see examples of sequencers and pulses coming up all the time, I am still a bit skeptical if we can find the right abstraction to make it indeed as simple as the example you sketch here.

This wasn't meant to be an abstraction, rather a specific example of functionality in our in-house DAC that I'm working on a driver for right now, which has some, as you say, very simple pulsing capabilities built in. I think you're right that there are going to be enough variations on how you have to specify pulses/sequences that a single abstraction is either a bad or impossible idea.

alexcjohnson commented 8 years ago

closed what's left of this with #55