Skip to content
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

Parameter improvements #600

Open
nulinspiratie opened this issue May 8, 2017 · 6 comments
Open

Parameter improvements #600

nulinspiratie opened this issue May 8, 2017 · 6 comments
Assignees

Comments

@nulinspiratie
Copy link
Contributor

Working with parameters, I have noticed there are some features missing that I think could be a useful addition.
I have listed them below.
The suggested improvements are mainly in the StandardParameter and the CombinedParameter.
Please let me know what suggestions you agree/disagree with, and possible ways (and pitfalls!) of implementing it.

General

Add get/set modifier

The val_mapping functionality is useful, because it allows the input to be transformed before performing the actual set/get.
The limitation here is that val_mapping must be a dict, and an enhancement would be to also accept functions for getting/setting.
This could be especially useful for the CombinedParameter, where it would allow you to use a single value to set a linear combination of several parameters.

Move delay to BaseParameter

Parameters other than the StandardParameter might need a delay.
It would therefore be good to standardize the set method to always perform a delay if the attribute is not set to None.
This could furthermore be used for inter_delay (see below).
I'm not sure what would be the best approach for this.
One idea would be to add it to the BaseParameter, which then decorates the set and get commands to include delays etc.
Another way is to create a method such as _set_with_delay or _set_decorated_, which is triggered by call(), and which then calls a set and performs delays.

Parameter

Default slice step

Being able to use the slice notation for parameters is very useful, as it is a quick way to create an np.arange list.
Currently, the slice notation requires three inputs: [start:stop:step].
However, both the default slice notation and np.arange don't need the step argument, and use the default value 1.
To mimick this behaviour, I suggest we also use 1 as a default value for step.

Accept numpy arrays for sweep points

When creating sweep values for a parameter (param[val_list]), val_list must be a list.
Often one would like to use values through numpy functions, such as arange or linspace.
Currently, parameters do not accept numpy arrays for sweep values, and so these arrays must first be converted to lists.
I suggest we also allow numpy arrays as sweep values.
As a side question, would this then need to be converted to a list internally?

StandardParameter

Replacing methods by dependent properties

Several methods are strongly related to getting/setting attributes (e.g. get_delay, set_delay).
These can be replaced dependent attributes (via the @Property decorator).
This results in more readible code, and less methods cluttering up the namespace.

Useless max_delay

From looking at the code, it seems to me that max_delay is not doing anything.
I've done some testing, and could not come up with a situation where max_delay actually changes something.
It is also not clear to me from the documentation what its use is.
Is someone using it, or does someone understand its use?
If not I suggest we remove it.

Change delay to post_delay, and add inter_delay

Currently, the attribute delay results in a delay after every set operation.
The main use of this is to let the system settle after such a set operation.

In some cases, a different type of delay can be useful, namely to ensure that two set operations are not performed too rapidly after each other.
One possible reason for this is that sending visa commands too rapidly could freeze up a device.
In this case, the current delay results in an unnecessary delay after each set operation.
To illustrate this, assume an instrument parameter that can only process one command per second.
After setting the parameter, an acquisition can be performed, which we assume also takes a second.
If we use the current delay, it will ensure that no two commands are sent within a second after each other.
However, setting a parameter and then subsequently performing an acquisition will take a total of two second instead of one.
This is because it will sleep for a second after setting the parameter, while it should be able to perform the acquisition straight away.

I suggest we change the current delay to post_delay, indicating that it will perform the delay after the set operation.
Adding a second inter_delay can be used to ensure that set operations are not performed too rapidly after each other.
This delay is not performed right after each set, but instead occurs when a subsequent set is performed too rapidly.
In this case, the system sleeps until inter_delay has passed, after which it will perform the subsequent set.

Change _validate_and_set and _validate_and_sweep to set

The two methods have a lot of overlapping code.
Furthermore, adding validate to the method name seems unnecessary (the ManualParameter.set does this implicitly)
I think it would be better to combine these two methods in the set method, which starts with checking if delay is None

CombinedParameter

Make CombinedParameter an actual parameter

Instead of being a Parameter, the CombinedParameter seems to have an attribute called parameter that somehow mimicks the behaviour of a parameter.
I'm not seeing why it can't simply be a parameter, either a subclass of Parameter, MultiParameter, or BaseParameter.
The comments of @giulioungaretti say that it's a hack, so could you explain why this hack is needed?
I suggest we transform it into a parameter.
We could then create a separate SweepValues class for the CombinedParameter.

Default value for name

The CombinedParameter currently requires a name.
This means that when using combine(param1, param2, ...), it is always necessary to also include a name.
Since the name usually refers to the parameters that are combined, we can have name as an optional argument, and have a default name.
This could be '{param1.name}{param2.name}...' or 'combined({param1.name}, {param2.name})' or something similar.

Add get method

There is no get method for the CombinedParameter, while in many cases this would be useful.
For instance, if you have two voltage gates which you want to treat as one.
There are several ways in which a get method can be implemented.
I think the best solution would be to by default return a set containing values for each of the parameters.
This default behaviour can be overridden by a get_modifier (see above).

Add allow_scalar

One useful feature of the CombinedParameter is to treat several parameters as one.
To this end, it would be useful to be able to set multiple parameters through a single value, i.e. combined_parameter(val) would set each of its parameter to val.
We could therefore add a flag allow_scalar, which for safety reasons should be set to False by default

Remove _aggregate method

We should be able to simplify this by simply using self.aggregate = aggregator

Modify set to receive values instead of index

By doing this, we can use the CombinedParameter while not necessraily in a loop

Add __getitem__ notation

This probably ties in with making the CombinedParameter a parameter.
This way, we won't have to use the combined_parameter.sweep() method, which isn't necessary for other parameter types.

@MerlinSmiles
Copy link
Contributor

@nulinspiratie Great additions you point out here!
I have one more.

Add a gain factor to a parameter

Sometimes instruments return values in not SI units, but rather mV or nA or something, it would simplify the code and make it more readable if a parameter had a gain one could use on numbers. That would translate the set and get values of that parameter, now we have to write an extra function into the driver to do that conversion.

@nulinspiratie
Copy link
Contributor Author

@MerlinSmiles Good idea, I would definitely be for this! This way, we wouldn't have to create a new parameter just for a conversion factor.

@giulioungaretti
Copy link
Contributor

@nulinspiratie one thing I can already say is that the slicing syntax will be most likely removed in the next version see #465 .

Let me carefully read the rest.

@giulioungaretti giulioungaretti self-assigned this May 9, 2017
@giulioungaretti
Copy link
Contributor

Working with parameters, I have noticed there are some features missing that I think could be a useful addition.
I have listed them below.
The suggested improvements are mainly in the StandardParameter and the CombinedParameter.
Please let me know what suggestions you agree/disagree with, and possible ways (and pitfalls!) of implementing it.

General

Add get/set modifier

Sounds really good !

Move delay to BaseParameter

It could be an idea to move all the delay things to the base parameter indeed.
I know @MerlinSmiles had some complaints//issues with the current way delays are handled.

Parameter

Default slice step

the slicing syntax will be axed, see comment before.

Accept numpy arrays for sweep points

The reason is the slice notation hack. The solution is to actually make the base parameter sweep method to accept an array other than the start, stop, step .

StandardParameter

Replacing methods by dependent properties

Yes, please.

Useless max_delay

See above, maybe even better we should have a joint effort making the entire delay thing better.

Change delay to post_delay, and add inter_delay

See above

Change _validate_and_set and _validate_and_sweep to set

Yes it would be a lot nicer and less code too!

CombinedParameter

Make CombinedParameter an actual parameter

The reason was just time pressure :D So yeah we can definitely improve!

Default value for name

As per above, sounds good!

actually @nulinspiratie : let us split this issue into two:

  • let's fix the param as per above comments (use this issue)

  • write a spec for the combined parameter ( this will also help in making it compatible with the future dataset and loops)

@jenshnielsen
Copy link
Collaborator

The issues with multi/array parameters noted in #498 are also worth remembering i.e.

  • Setpoints are complicated and hard to use for nD arrays
  • Multi/arrayparamers cannot be set at all in a loop

@nulinspiratie
Copy link
Contributor Author

@giulioungaretti good idea to split the issue into two. I think the CombinedParameter will have a few hurdles to implement, so first creating specs is a good idea. I'll create one, should that be a PR? I have also begun with the easiest changes to parameters, as they are quite straightforward to implement. I'll create a PR once they are done, and we can continue from there to tackle the more complicated changes.

@jenshnielsen I wasn't aware of the issues with the MultiParameter, I'll have a look at them

nulinspiratie added a commit to nulinspiratie/Qcodes that referenced this issue May 16, 2017
Remove max_delay because it did not seem to work.
For more info see bullet point microsoft#600.

This breaks code that used max_delay
This was referenced Sep 25, 2017
WilliamHPNielsen pushed a commit that referenced this issue Sep 29, 2017
* refactor: Remove max_delay

Remove max_delay because it did not seem to work.
For more info see bullet point #600.

This breaks code that used max_delay

* Refactor: Change get/set_delay to dependent properties

* Refactor: Change get/set_step to dependent property

Additional changes
- max_val_age is temporarily useless (to be moved to GetLatest)
- creating set val temporarily moved to init

max_val_age temporarily unusable

* Refactor: Delay -> Post_delay

* Refactor: Merge _validate_and_set/sweep to set

Set now handles both cases

* Refactor: rename _set_get/set to _initialize_get/set

Naming was confusing

* Refactor: remoed _update_last_ts

Was unnecessary

* Refactor: Moved max_val_age to base_parameter and GetLatest()

Max val age is now always checked when get_latest is asked.
A consequence is that parameter.get_latest will perform a get() if the last get was performed more than max_val_age ago.

* fix: Removed unnecessary quote

* refactor: rename _get/_set to _get_command/_set_command

* fix: GetLatest now performs get on parameter

* refactor: removed has_set/has_get

* Add get/set wrappers

* feat: add kwargs to set from call()

* refactor: move step to base_parameter, re-add exception to StandardParameter.get()

Accidentally removed exception in a previous commit

* feat: Added inter_delay, post_delay

Still need to update docstrings

* refactor: moved _initialize_get/set to parameter init, changed _vals to vals

* refactor: Removed set_validator

* refactor: validate to BaseParameter, get/set_cmd also handle None

Other parameters, such as MultiParameter, may also need some sort of validation.
Will see in the future if validate may instead be an abstract base method that should be overridden
When get_cmd is None and .get is not set, it will be equal to get_latest
When set_cmd is None and .set is not set, it will be equal to saving the value.
This behaviour is equal to the ManualParameter

* move initial_val to Parameter, remove ManualParameter

* fix: Subclass InstrumentRefParamter from Parameter

* feat: add valmapping to BaseParameter, remove StandardParameter

* get/set_parser now implemented in BaseParameter get/set wrappers

* refactor: removal of units in (Array)Parameter

* feat: add scale

* Added StandardParameter and Manualparameter as deprecated

* feat: added raw_value

* fix: Placed deprecation warnings after super().__init__

* refactor: change _get/set_wrapper to _wrap_get/set

* fix: get/set after super().__init__ to ensure all attrs are set

Travis was failing since get_latest was not yet defined

* refactor: replace full_name by str(parameter)

* refactor: remove get_attrs, replace full_name occurrences

* fix: aggregator typo

* fix: replace is_sequence Iterator by Iterable

Doing this enables numpy arrays to be included as sweep values

* refactor: change InstrumentRefParameter set_validators

* refactor: combine _latest_value, _latest_ts by _latest dict attr, remove _latest method

* refactor: _save_val validates by default

* fix: remove partial import

* refactor: remove no_getter/setter, unused imports

* refactor: tidy up code, remove todo

* refactor: change back save_val by default to validator=False

Forgot that it was used by other custom parameters, and so changing default behaviour might lead to issues

* fix: snapshot does not change self._latest

* feat: only include meta_attrs that are not None

* fix: vals in base_parameter

* fix: raise error when max_val_age is set but no get/get_cmd

* fix: move setting of vals attr earlier

* fix: Multiparameter accepts array as setpoints

* fix: change setter values to value

* fix: allow step=None

* fix: setpoints in ArrayParameter can now be an np.array

* doc: remove trailing whitespaces

* fix: docstring typo

* fix: allow step=None

* fix: functools wraps function wraps get/set fuction

* fix: instrument with empty name not included in full_name

* refactor: change get_sweep_values to get_ramp_values

* fix: change add_parameter default parameter, ramping fix

* fix: reverted Iterable to Iterator, explicitly added np.ndarray

* fix: Forgot to add helpers to previous commit

* correct conflict resolution

all these methods have been moved to the instrumentbase class

* fix: replaced reference to `has_get`, `has_set`

* test: fixed testParameter tests

* test:fix most other tests

* fix: CombinedParameter defaults to set_cmd=None

* fix: all existing tests are working

* fix: get_ramp_values also returns final value

* test: add test for step, ramp, scale, raw_value

* test: add test for GetLatest, including max_val_age

* test: fix combined tests

* test: fix test_loop error

* docs: update global docstring

* docs: start with BaseParameter docs

* docs: finish BaseParameter

* docs: finish Parameters docs

* fix: defaults for snapshot_get, metadata

* fix: set vals to Enum if val_mapping provided

* feat: Remove default vals=Numbers() for Parameter

* fix: remove unused import

* doc: fixes

* forgot call to super().__init__()

* doc: unindent

* doc: remove trailing whitespaces

* added super.__init__(None)

* doc: damn white spaces

* fix: remove init from getlatest

* fix: test fix for vals=Numbers()

* readd deprecated set_validator

this will be removed after a deprecation cycle

* use warnings module for deprecation warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants