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

add_parameter() with DelegateParameter have inconsistent defaults for bind_to_instrument #6715

Open
thangleiter opened this issue Dec 12, 2024 · 4 comments · May be fixed by #6722
Open

add_parameter() with DelegateParameter have inconsistent defaults for bind_to_instrument #6715

thangleiter opened this issue Dec 12, 2024 · 4 comments · May be fixed by #6722

Comments

@thangleiter
Copy link
Contributor

Using the preferred pattern param = inst.add_parameter(...) together with param_cls=DelegateParameter defaults to bind_to_instrument=True, whereas DelegateParameter defaults to False. The latter's behavior is explicitly pointed out in the docs (which also makes sense to me).

However, forcing bind_to_instrument=False raises a QCoDeSDeprecationWarning.

MWE:

>>> from qcodes.instrument import Instrument
>>> from qcodes.parameters import DelegateParameter
>>> inst = Instrument('foo')
>>> param = inst.add_parameter('bar')
>>> delegate = inst.add_parameter('baz', DelegateParameter, source=param)
>>> inst.parameters
{'IDN': <qcodes.parameters.parameter.Parameter: IDN at 1349665492432>,
 'bar': <qcodes.parameters.parameter.Parameter: bar at 1349689473456>,
 'baz': <qcodes.parameters.delegate_parameter.DelegateParameter: baz at 1349691204784>}
>>> inst.remove_parameter('baz')
>>> delegate = inst.add_parameter('baz', DelegateParameter, source=param, bind_to_instrument=False)
2024-12-12 14:22:37,344 ¦ py.warnings ¦ WARNING ¦ warnings ¦ _showwarnmsg ¦ 112 ¦ ...\src\qcodes\instrument\instrument_base.py:184: QCoDeSDeprecationWarning: Parameter baz did not correctly register itself on instrument foo. Please check that `instrument` argument is passed from <class 'qcodes.parameters.delegate_parameter.DelegateParameter'> all the way to `ParameterBase`. This will be an error in the future.
  warnings.warn(

At the very least, the inconsistency should be resolved, however, I'd petition for allowing DelegateParameters not bound to instruments.

@jenshnielsen
Copy link
Collaborator

@thangleiter There is a logical error in add_parameter if bind_to_instrument is False. I am however curious of you use case for using add_parameter if you don't want to bind to the instrument? Why not simply create the Parameter with DelegateParameter("name", ...) for that use case? The only thing add_parameter really does is to create the parameter with the supplied args and then registers it on the instrument.

@thangleiter
Copy link
Contributor Author

Hi @jenshnielsen, in the end it's not a crucial issue. It just came up as I was updating the TimeTagger driver to use the preferred pattern for adding parameters of using add_parameter and assigning the return value. That driver has some hacky helper parameters that don't need to be snapshotted or anything:
https://github.com/QCoDeS/Qcodes_contrib_drivers/blob/438417c01137b5a87adc0c5b67680e8fa09703f5/src/qcodes_contrib_drivers/drivers/SwabianInstruments/Swabian_Instruments_Time_Tagger.py#L297-L305

Obvioiusly, I could just not use add_parameter, but I think it's ill-advised to proliferate both the param = Parameter(...) and param = self.add_parameter(...) patterns now that there's finally a solution that combines the two patterns that existed alongside previously.

@jenshnielsen
Copy link
Collaborator

@thangleiter Thanks for the explanation.

If you want to avoid snapshotting I would recommend passing snapshot_exclude=True to add_parameter

Honestly, I never planned for the use case where someone would do `self.foo = self.add_parameter(..., bind_to_instrument=False) and the intension was that if a parameter was assigned as an attribute, it should also be bound to the instrument (which basically amount to adding it to the parameters dict.

I would be tempted to suggest that:
We fix the bug such that add_parameter with bind_to_instrument=False works correctly but make that behaviour warn and recomed not doing that highlighting snapshot_exclude as an alternative. Would that work for you?

@thangleiter
Copy link
Contributor Author

That should work, yes.

@jenshnielsen jenshnielsen linked a pull request Dec 16, 2024 that will close this issue
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants