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.

Sign up for GitHub

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

Jump to bottom

Parameter.__getitem__ behavior #6080

Open
thangleiter opened this issue May 16, 2024 · 5 comments · Fixed by #6084
Open

Parameter.__getitem__ behavior #6080

thangleiter opened this issue May 16, 2024 · 5 comments · Fixed by #6084

Comments

@thangleiter
Copy link
Contributor

Parameters can be indexed to return SweepFixedValues. This is a handy shortcut if you know it exists and use it correctly. However, it can lead to extremely unpredictable behavior when parameters are used in the wrong context, for example if a Parameter is passed to a function that expects an Iterable.

The function might try to convert the Iterable to a list and calls list(param), which runs forever if the parameter's validator passes all positive integers. In my opinion, this is very unhandy behavior and not worth the syntactic sugar of writing param[:10] instead of having a method that achieves the same thing without the unintended consequences of making Parameter iterable.

An example, where the unassuming user passes a Parameter instead of a sequence of parameters as setpoints to Measurement.register_parameter:

from qcodes.parameters import ManualParameter, ParameterWithSetpoints
from qcodes.validators import Arrays
from qcodes.dataset import Measurement

getme = ManualParameter('foo')
setme = ManualParameter('bar')

meas = Measurement()
meas.register_parameter(setme)
meas.register_parameter(getme, setpoints=setme)
# should error, since setpoints should be Sequence[Union[str, "ParameterBase"]]
# instead, raises obscure error
AttributeError: 'SweepFixedValues' object has no attribute 'register_name'

For ParameterWithSetpoints, register_parameter ends up trying to convert the argument to a list:

getme_setpoints = ParameterWithSetpoints('baz', setpoints=(setme,), vals=Arrays(shape=(10,)))

meas = Measurement()
meas.register_parameter(setme)
meas.register_parameter(getme_setpoints, setpoints=setme)
# should error, instead runs forever

In this case, it would even be cumbersome to check in _register_parameter_with_setpoints() if setpoints is the correct type, i.e., , because iter(setpoints) works even if setpoints is a Parameter! (*Well, Parameter is not an instance of Sequence, so not too cumbersome.)

Also runs forever:

for _ in setme:
    pass

My proposal is therefore to deprecate Parameter.__getitem__ and instead introduce a method like Parameter.sweep(keys: int | slice) or even mirror xarray and call it Parameter.iloc or something.
@jenshnielsen
Copy link
Collaborator

It should indeed be deprecated see #5036

__getitem__ and parameter.sweep (which already exists) are both remaining leftovers of qcodes_loop in qcodes that I have not yet found a clean way to get rid of without breaking that package.

We are not planning to introduce any sweep functionality into the parameter to use with qcodes.dataset but parameters can be sweep by passing them to LinSweep and friends. This choice is made since

  • The parameter class is already too complicated and does too many things
  • Having the sweeper externally gives greater flexibility in implementing different sweep strategies.

I however, agree that it would be great to have better validation in register_parameter to error early if setpoints is a ParameterBase subclass. A pr for that is most welcome.

@thangleiter
Copy link
Contributor Author

Ah, apologies for not searching for an existing issue. I don't actually use the functionality, so I'm all for keeping Parameter simple.

I can open up a PR for validation in register_parameter, but it's only putting out one small fire compared to the proper solution in #5036.

@thangleiter thangleiter mentioned this issue May 17, 2024
Merged
@jenshnielsen jenshnielsen mentioned this issue May 17, 2024
Open
3 tasks
@jenshnielsen jenshnielsen reopened this May 20, 2024
@jenshnielsen
Copy link
Collaborator

@thangleiter I think we should reopen since we have only resolved this partially? Do you agree

@thangleiter
Copy link
Contributor Author

@thangleiter I think we should reopen since we have only resolved this partially? Do you agree

I guess it makes sense to keep it open for bookkeeping. The last of the examples above is indeed not fixed by #6084.

@jenshnielsen
Copy link
Collaborator

Thinking a bit more about this my proposed solution is.

  • Add sweep and __getitem__ above to a new Sweeper class in qcodes-loop and update all of that pacakge to make use of this.
  • Cut a new release of qcodes-loop
  • Deprecate these two methods

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Test setpoints and basis types in register_parameter qutech/Qcodes
2 participants
@jenshnielsen @thangleiter