Skip to content

Bugfixing Parameters Interface & Adding a Callback to VoltageControlComponent#37

Open
Ulfgard wants to merge 5 commits into
qua-platform:mainfrom
Ulfgard:parameters
Open

Bugfixing Parameters Interface & Adding a Callback to VoltageControlComponent#37
Ulfgard wants to merge 5 commits into
qua-platform:mainfrom
Ulfgard:parameters

Conversation

@Ulfgard

@Ulfgard Ulfgard commented Jul 4, 2025

Copy link
Copy Markdown
Collaborator

This PR adds a few bug fixes and improvements to Parameters:

  1. the ParameterProtocol now mirrors exactly QCodes Parameters and fix a few smaller follow-up problems. A result of this is that some function are renamed. I ensured backwards compatibility for naive usage by keeping constructor parameter names the same (e.g. the member variable name changed units->unit, but the old parameter name "units" is kept in the constructor.
  2. added and fixed static type checking to BasicParameter. This scheme can be mirrored in any other file in the repo.
  3. the delay in BasicParameter is now configurable and zero by default to make it more general

Further, it adds a new feature which allows VoltageControlComponent callback if any of the parameters is changed via ParameterProtocol.set(). This is necessary because we might want to inform the data acquirer if any of the parameters are changed. for example a change of a plunger gate could trigger deletion of history. The Implementation is done via a new class CallbackParameter which just wraps ParameterProtocol.set() and calls the callback with the changed parameter provided as argument. This seemed to be the least invasive way. TypeChecking was also added here.

@CLAassistant

CLAassistant commented Jul 4, 2025

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@nulinspiratie nulinspiratie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice pull request. It looks good. A few thoughts on this.

First, good catch on unit versus units. I'm wondering whether we should make the function signature use unit by default, and then in keyword arguments, also check for units if it's passed to add backwards compatibility. We could add a deprecation warning for this.

Second, for the callback parameter, I think it makes sense as well. Maybe the callback should receive three arguments: one is the parameter, second is the previous value (using get_latest()), and the third is the new value. It should get the latest value before, then update, and then do the callback. This way, you can see both the old value and the new value, which might be useful information for the callback.

Given the simulated delay in the basic parameter might not be that important, maybe we should just remove the delay. If it's really a basic parameter, I don't think it needs it. It was mainly used for testing purposes.

Finally, I don't fully understand the check property you have in type checking. Can you explain this part?

Let me know what you think. I don't feel strongly on any of the points raised above

@Ulfgard

Ulfgard commented Jul 10, 2025

Copy link
Copy Markdown
Collaborator Author
  1. I think supporting both, with a depreciation warning for the second is okay. I do not want to fiddle around with kwargs, i would just add units=None and then check inside whether it is not None and warn then.
  2. Good idea. I think two parameters are enough, since parameter.get_latest() already returns the new value. i think we can make it clear by calling the second parameter "previous_value".
  3. I felt that much. We can remove it altogether, I think.
  4. The check property is something i learned only recently. if TYPE_CHECKING: only executes when a static type checker is run. In that case check() exists and gets analyzed by the type checker, which means that it tries to instantiate the checked class with the given parameter types and assign it to the given typed object, which forces the type check for the Protocol.

@Ulfgard

Ulfgard commented Jul 10, 2025

Copy link
Copy Markdown
Collaborator Author

changes are incorporated

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 this pull request may close these issues.

3 participants