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 vals property and setter to delegate_parameter #6817

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/changes/newsfragments/6796.improved
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add vals property and @vals.setter to qcodes.parameters.delegate_parameter.DelegateParameter to propagate the Validator from source to DelegateParameter.
23 changes: 23 additions & 0 deletions src/qcodes/parameters/delegate_parameter.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
from collections.abc import Sequence
from datetime import datetime

from qcodes.validators import Validator

from .parameter_base import ParamDataType, ParamRawDataType


Expand Down Expand Up @@ -171,9 +173,11 @@ def __init__(

initial_cache_value = kwargs.pop("initial_cache_value", None)
self.source = source
self._vals_override: Validator | None = None
super().__init__(name, *args, **kwargs)
self.label = kwargs.get("label", None)
self.unit = kwargs.get("unit", None)
self.vals = kwargs.get("vals", None)

# Hack While we inherit the settable status from the parent parameter
# we do allow param.set_to to temporary override _settable in a
Expand Down Expand Up @@ -223,6 +227,23 @@ def unit(self) -> str:
def unit(self, unit: str | None) -> None:
self._unit_override = unit

@property
def vals(self) -> Validator | None:
"""
The validator of the parameter. Read from source if not explicitly overwritten.
Set to None to disable overwrite.
"""
if self._vals_override is not None:
return self._vals_override
elif self.source is not None:
return self.source.vals
else:
return None
Copy link
Collaborator

Choose a reason for hiding this comment

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

ParameterBase supports multiple validators via the add_validator etc logic. We need to consider if we want to support that here too.


@vals.setter
def vals(self, vals: Validator | None) -> None:
self._vals_override = vals

@property
def label(self) -> str:
"""
Expand Down Expand Up @@ -314,3 +335,5 @@ def validate(self, value: ParamDataType) -> None:
super().validate(value)
if self.source is not None:
self.source.validate(self._from_value_to_raw_value(value))
if self.vals is not None:
self.vals.validate(value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary. Will the call to super.validate above not do the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it will only validate with the validator on the source, so it will not enforce the validator on the delegate parameter without this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The additional cases I added in the tests show this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see what is going on. the call to validator in the base class will use all validators stored as part of _vals. Since this pr prevents you from adding any validator to that list it will always be empty so that line does nothing on this pr. However prior to this pr adding a validator to the DelegateParameter would cause it to be added to the parameters _vals list and used. This makes me suspect that the tests as is would also pass on main. #6832 seems to confirm this too so I am not completely sure if this pr is required

32 changes: 32 additions & 0 deletions tests/parameter/test_delegate_parameter.py
Original file line number Diff line number Diff line change
Expand Up @@ -574,18 +574,23 @@ def test_value_validation() -> None:
source_param = Parameter("source", set_cmd=None, get_cmd=None)
delegate_param = DelegateParameter("delegate", source=source_param)

# Test case where source parameter validator is None and delegate parameter validator is
# specified.
delegate_param.vals = vals.Numbers(-10, 10)
source_param.vals = None
delegate_param.validate(1)
with pytest.raises(ValueError):
delegate_param.validate(11)

# Test where delegate parameter validator is None and source parameter validator is
# specified.
delegate_param.vals = None
source_param.vals = vals.Numbers(-5, 5)
delegate_param.validate(1)
with pytest.raises(ValueError):
delegate_param.validate(6)

# Test case where source parameter validator is more restricted than delegate parameter.
delegate_param.vals = vals.Numbers(-10, 10)
source_param.vals = vals.Numbers(-5, 5)
delegate_param.validate(1)
Expand All @@ -594,6 +599,33 @@ def test_value_validation() -> None:
with pytest.raises(ValueError):
delegate_param.validate(11)

# Test case that the order of setting validator on source and delegate parameters does not matter.
source_param.vals = vals.Numbers(-5, 5)
delegate_param.vals = vals.Numbers(-10, 10)
delegate_param.validate(1)
with pytest.raises(ValueError):
delegate_param.validate(6)
with pytest.raises(ValueError):
delegate_param.validate(11)

# Test case where delegate parameter validator is more restricted than source parameter.
delegate_param.vals = vals.Numbers(-5, 5)
source_param.vals = vals.Numbers(-10, 10)
delegate_param.validate(1)
with pytest.raises(ValueError):
delegate_param.validate(6)
with pytest.raises(ValueError):
delegate_param.validate(11)

# Test case that the order of setting validator on source and delegate parameters does not matter.
source_param.vals = vals.Numbers(-10, 10)
delegate_param.vals = vals.Numbers(-5, 5)
delegate_param.validate(1)
with pytest.raises(ValueError):
delegate_param.validate(6)
with pytest.raises(ValueError):
delegate_param.validate(11)


def test_value_validation_with_offset_and_scale() -> None:
source_param = Parameter(
Expand Down
Loading