-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
optional arguments to Functions #51
Comments
@alexcjohnson , I am not entirely convinced of the advantage of the add Function functionality The advantages you list are as follows
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.
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.
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.
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. |
It's true, it's not nearly as useful as
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 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 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))
fair points. |
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
I like this 👍 , 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.
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. |
Good call - I'll just put it in validators.py
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. |
closed what's left of this with #55 |
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 aParameter
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 aParameter
is and how to use it) and for our core code (which then only ever has to handle a single value when setting aParameter
)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 explicitFunction
object via theadd_function
method, but the advantages ofadd_function
are:self.functions
in case a user wants to list all the instrument functionality, separate from the base instrument methodsSo with that in mind, how do we implement optional arguments in
Function
? Right now the arguments are specified as a list ofValidator
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
andFunctions
anyway per #42 andFunction
used to call theseparameters
but that's obviously confusing vsParameter
- so I changed them toargs
)The text was updated successfully, but these errors were encountered: