diff --git a/docs/changes/newsfragments/8451.breaking b/docs/changes/newsfragments/8451.breaking new file mode 100644 index 00000000000..02be0b926ae --- /dev/null +++ b/docs/changes/newsfragments/8451.breaking @@ -0,0 +1,14 @@ +Fixed applying a scalar ``scale`` or ``offset`` when setting a parameter to a +sequence such as a ``list`` or a ``tuple``. Multiplying a sequence by a number +repeats it rather than scaling its elements, so ``param([10, 20])`` with +``param.scale = 2`` used to set the raw value to ``[10, 20, 10, 20]``, and a +scalar ``offset`` raised a ``TypeError``. Sequences are now converted element +wise, matching how the values are converted back when the parameter is read. +Numpy arrays are unaffected since they scale and offset element wise already. + +Additionally, applying a ``scale`` or ``offset`` that does not match the length +of the value now raises a ``ValueError``. Previously the value and the +scale/offset were zipped together without checking their lengths, so a mismatch +silently dropped the extra elements, e.g. setting a parameter with +``scale = [2, 4]`` to ``[10, 20, 30]`` used to set the raw value to ``(20, 80)``. +See :pr:`8450`. diff --git a/src/qcodes/parameters/parameter_base.py b/src/qcodes/parameters/parameter_base.py index d20f1e27d15..16d96c8b08e 100644 --- a/src/qcodes/parameters/parameter_base.py +++ b/src/qcodes/parameters/parameter_base.py @@ -2,6 +2,7 @@ import collections.abc import logging +import operator import time import warnings from collections.abc import Iterator, MutableSet @@ -265,11 +266,64 @@ class ParameterBaseKWArgs( # generic ``ParameterDataTypeVar`` out of the arithmetic. +_CONVERSION_KIND: dict[Callable[[Any, Any], Any], str] = { + operator.mul: "scale", + operator.truediv: "scale", + operator.add: "offset", + operator.sub: "offset", +} + + +def _apply_elementwise( + value: Any, + conversion: Iterable[float], + operation: Callable[[Any, Any], Any], +) -> tuple[Any, ...]: + """Combine ``value`` and ``conversion`` element by element. + + Args: + value: The (iterable) value to convert. + conversion: The scale or offset to apply, one element per value. + operation: The arithmetic operation to apply to each pair of elements. + The operation also determines the name of the conversion used in + the error message. + + Returns: + The converted values. + + Raises: + ValueError: If ``value`` and ``conversion`` are of different length. + + """ + try: + return tuple( + operation(val, sub_value) + for val, sub_value in zip(value, conversion, strict=True) + ) + except ValueError as err: + kind = _CONVERSION_KIND[operation] + raise ValueError( + f"Cannot apply {kind} of length {_length_for_error(conversion)} " + f"to a value of length {_length_for_error(value)}." + ) from err + + +def _length_for_error(obj: Any) -> str: + """The length of ``obj`` for use in an error message.""" + if isinstance(obj, collections.abc.Sized): + return str(len(obj)) + return "unknown" + + def _scale_raw_value(raw_value: Any, scale: float | Iterable[float]) -> Any: """Multiply a value by ``scale`` on the way to the instrument.""" if isinstance(scale, collections.abc.Iterable): # Scale contains multiple elements, one for each value - return tuple(val * sub_scale for val, sub_scale in zip(raw_value, scale)) + return _apply_elementwise(raw_value, scale, operator.mul) + if isinstance(raw_value, collections.abc.Sequence): + # Multiplying a sequence by a number repeats it rather than + # scaling its elements, so these must be handled element wise. + return tuple(val * scale for val in raw_value) # Use single scale for all values return raw_value * scale @@ -278,7 +332,11 @@ def _offset_raw_value(raw_value: Any, offset: float | Iterable[float]) -> Any: """Add ``offset`` to a value on the way to the instrument.""" if isinstance(offset, collections.abc.Iterable): # offset contains multiple elements, one for each value - return tuple(val + sub_offset for val, sub_offset in zip(raw_value, offset)) + return _apply_elementwise(raw_value, offset, operator.add) + if isinstance(raw_value, collections.abc.Sequence): + # Adding a number to a sequence is an error, so these must be + # handled element wise. + return tuple(val + offset for val in raw_value) # Use single offset for all values return raw_value + offset @@ -290,7 +348,7 @@ def _unoffset_value(value: Any, offset: float | Iterable[float]) -> Any: except TypeError: if isinstance(offset, collections.abc.Iterable): # offset contains multiple elements, one for each value - return tuple(val - sub_offset for val, sub_offset in zip(value, offset)) + return _apply_elementwise(value, offset, operator.sub) elif isinstance(value, collections.abc.Iterable): # Use single offset for all values return tuple(val - offset for val in value) @@ -305,7 +363,7 @@ def _unscale_value(value: Any, scale: float | Iterable[float]) -> Any: except TypeError: if isinstance(scale, collections.abc.Iterable): # Scale contains multiple elements, one for each value - return tuple(val / sub_scale for val, sub_scale in zip(value, scale)) + return _apply_elementwise(value, scale, operator.truediv) elif isinstance(value, collections.abc.Iterable): # Use single scale for all values return tuple(val / scale for val in value) diff --git a/tests/parameter/test_parameter_scale_offset.py b/tests/parameter/test_parameter_scale_offset.py index 4350d1c5a92..7ba9533c668 100644 --- a/tests/parameter/test_parameter_scale_offset.py +++ b/tests/parameter/test_parameter_scale_offset.py @@ -261,6 +261,63 @@ def test_set_numpy_array_with_scalar_scale_and_offset() -> None: np.testing.assert_allclose(param.get(), [10, 20]) +@pytest.mark.parametrize("container", [list, tuple]) +def test_set_sequence_with_scalar_scale_and_offset(container: type) -> None: + """A list or tuple must be scaled element wise, not repeated.""" + param = Parameter(name="test_param", set_cmd=None, get_cmd=None) + param.scale = 2 + param.offset = 1 + + param(container([10, 20])) + + assert param.raw_value == (21, 41) + assert param.get() == (10, 20) + + +@pytest.mark.parametrize("container", [list, tuple]) +def test_set_sequence_with_scalar_scale_only(container: type) -> None: + param = Parameter(name="test_param", set_cmd=None, get_cmd=None) + param.scale = 2 + + param(container([10, 20])) + + assert param.raw_value == (20, 40) + assert param.get() == (10, 20) + + +@pytest.mark.parametrize("container", [list, tuple]) +def test_set_sequence_with_scalar_offset_only(container: type) -> None: + param = Parameter(name="test_param", set_cmd=None, get_cmd=None) + param.offset = 1 + + param(container([10, 20])) + + assert param.raw_value == (11, 21) + assert param.get() == (10, 20) + + +def test_set_sequence_with_scalar_scale_and_iterable_offset() -> None: + param = Parameter(name="test_param", set_cmd=None, get_cmd=None) + param.scale = 2 + param.offset = [1, 2] + + param([10, 20]) + + assert param.raw_value == (21, 42) + assert param.get() == (10, 20) + + +def test_set_sequence_with_iterable_scale_and_scalar_offset() -> None: + param = Parameter(name="test_param", set_cmd=None, get_cmd=None) + param.scale = [2, 4] + param.offset = 1 + + param([10, 20]) + + assert param.raw_value == (21, 81) + assert param.get() == (10, 20) + + def test_get_sequence_with_scalar_scale_and_offset() -> None: """A list valued raw value falls back on element wise arithmetic.""" param = Parameter(name="test_param", set_cmd=None, get_cmd=lambda: [10, 20]) @@ -294,12 +351,20 @@ def test_get_raises_for_non_numeric_value(attribute: str) -> None: def test_scale_raw_value_helper() -> None: assert _scale_raw_value(10, 2) == 20 assert _scale_raw_value([10, 20], [2, 4]) == (20, 80) + assert _scale_raw_value([10, 20], 2) == (20, 40) + assert _scale_raw_value((10, 20), 2) == (20, 40) + # any sequence, not just list and tuple + assert _scale_raw_value(range(10, 30, 10), 2) == (20, 40) np.testing.assert_allclose(_scale_raw_value(np.array([10, 20]), 2), [20, 40]) def test_offset_raw_value_helper() -> None: assert _offset_raw_value(10, 2) == 12 assert _offset_raw_value([10, 20], [2, 4]) == (12, 24) + assert _offset_raw_value([10, 20], 2) == (12, 22) + assert _offset_raw_value((10, 20), 2) == (12, 22) + # any sequence, not just list and tuple + assert _offset_raw_value(range(10, 30, 10), 2) == (12, 22) np.testing.assert_allclose(_offset_raw_value(np.array([10, 20]), 2), [12, 22]) @@ -333,3 +398,95 @@ def test_helpers_do_not_mutate_their_input( value = [10.0, 20.0] helper(value, [2.0, 4.0]) assert value == [10.0, 20.0] + + +@pytest.mark.parametrize("attribute", ["scale", "offset"]) +def test_set_raises_on_length_mismatch(attribute: str) -> None: + """A scale/offset that does not match the length of the value is an error.""" + param = Parameter(name="test_param", set_cmd=None, get_cmd=None) + setattr(param, attribute, [2, 4]) + + with pytest.raises( + ValueError, + match=f"Cannot apply {attribute} of length 2 to a value of length 3", + ): + param([10, 20, 30]) + + +@pytest.mark.parametrize("attribute", ["scale", "offset"]) +def test_set_raises_on_length_mismatch_for_numpy_value(attribute: str) -> None: + param = Parameter(name="test_param", set_cmd=None, get_cmd=None) + setattr(param, attribute, [2, 4]) + + with pytest.raises( + ValueError, + match=f"Cannot apply {attribute} of length 2 to a value of length 3", + ): + param(np.array([10, 20, 30])) + + +@pytest.mark.parametrize("attribute", ["scale", "offset"]) +def test_get_raises_on_length_mismatch(attribute: str) -> None: + param = Parameter(name="test_param", set_cmd=None, get_cmd=lambda: [10, 20, 30]) + setattr(param, attribute, [2, 4]) + + with pytest.raises( + ValueError, + match=f"Cannot apply {attribute} of length 2 to a value of length 3", + ): + param.get() + + +@pytest.mark.parametrize("attribute", ["scale", "offset"]) +def test_get_raises_on_length_mismatch_for_numpy_value(attribute: str) -> None: + """Numpy raises its own error before the element wise fallback is reached.""" + param = Parameter( + name="test_param", set_cmd=None, get_cmd=lambda: np.array([10.0, 20.0, 30.0]) + ) + setattr(param, attribute, [2, 4]) + + with pytest.raises(ValueError, match="could not be broadcast together"): + param.get() + + +@pytest.mark.parametrize( + ("helper", "kind"), + [ + (_scale_raw_value, "scale"), + (_offset_raw_value, "offset"), + (_unoffset_value, "offset"), + (_unscale_value, "scale"), + ], +) +def test_helpers_raise_on_length_mismatch( + helper: Callable[[Any, Any], Any], kind: str +) -> None: + with pytest.raises( + ValueError, match=f"Cannot apply {kind} of length 2 to a value of length 3" + ): + helper([10, 20, 30], [2, 4]) + + with pytest.raises( + ValueError, match=f"Cannot apply {kind} of length 3 to a value of length 2" + ): + helper([10, 20], [2, 4, 6]) + + +@pytest.mark.parametrize( + ("helper", "kind"), + [ + (_scale_raw_value, "scale"), + (_offset_raw_value, "offset"), + (_unoffset_value, "offset"), + (_unscale_value, "scale"), + ], +) +def test_helpers_report_unknown_length_for_iterators( + helper: Callable[[Any, Any], Any], kind: str +) -> None: + """An iterable that has no length is reported as unknown.""" + with pytest.raises( + ValueError, + match=f"Cannot apply {kind} of length unknown to a value of length 3", + ): + helper([10, 20, 30], iter([2, 4]))