diff --git a/docs/changes/0.46.0.rst b/docs/changes/0.46.0.rst index e558d5b27d9..f15b2200f03 100644 --- a/docs/changes/0.46.0.rst +++ b/docs/changes/0.46.0.rst @@ -1,4 +1,4 @@ -QCoDeS 0.46.0 (2024-06-10) +QCoDeS 0.46.0 (2024-06-27) ========================== Breaking Changes: @@ -18,6 +18,12 @@ Breaking Changes: All drivers shipping with qcodes for Vendors from A-K have been updated in this pr. The remaining drivers were updated in (:pr:`6087`). (:pr:`6012`) +- It is now considered unsupported to modify the `parameters` attribute of an instrument or instrument module after it has been created. + To remove a parameter from an instrument use the `remove_parameter` method. (:pr:`6174`) + +- InstrumentBase.add_parameter will now error if an attribute of the same name as the parameter added already exists. This + is similar to how it would error if a parameter of the same name existed. (:pr:`6174`) + Improved: --------- diff --git a/src/qcodes/instrument/instrument_base.py b/src/qcodes/instrument/instrument_base.py index fe7100747d7..519b1ce66f9 100644 --- a/src/qcodes/instrument/instrument_base.py +++ b/src/qcodes/instrument/instrument_base.py @@ -186,6 +186,29 @@ def add_parameter( self.parameters[name] = param return param + def remove_parameter(self, name: str) -> None: + """ + Remove a Parameter from this instrument. + + Unlike modifying the parameters dict directly, this method will + make sure that the parameter is properly unbound from the instrument + if the parameter is added as a real attribute to the instrument. + If a property of the same name exists it will not be modified. + If name is an attribute but not a parameter, it will not be modified. + + Args: + name: The name of the parameter to remove. + + Raises: + KeyError: If the parameter does not exist on the instrument. + """ + self.parameters.pop(name) + + is_property = isinstance(getattr(self.__class__, name, None), property) + + if not is_property and hasattr(self, name): + delattr(self, name) + def add_function(self, name: str, **kwargs: Any) -> None: """ Bind one ``Function`` to this instrument. diff --git a/src/qcodes/parameters/parameter_base.py b/src/qcodes/parameters/parameter_base.py index 1cb10435718..6a0ee182d41 100644 --- a/src/qcodes/parameters/parameter_base.py +++ b/src/qcodes/parameters/parameter_base.py @@ -316,13 +316,28 @@ def __init__( self._abstract = abstract if instrument is not None and bind_to_instrument: - existing_parameter = instrument.parameters.get(name, None) + found_as_delegate = instrument.parameters.get(name, False) + # we allow properties since a pattern that has been seen in the wild + # is properties that are used to wrap parameters of the same name + # to define an interface for the instrument + is_property = isinstance( + getattr(instrument.__class__, name, None), property + ) + found_as_attr = not is_property and hasattr(instrument, name) + + if found_as_delegate or found_as_attr: + existing_parameter = instrument.parameters.get(name, None) - if existing_parameter: - if not existing_parameter.abstract: + if existing_parameter is not None and not existing_parameter.abstract: raise KeyError( f"Duplicate parameter name {name} on instrument {instrument}" ) + if existing_parameter is None: + existing_attribute = getattr(instrument, name, None) + if existing_attribute is not None: + raise KeyError( + f"Parameter {name} overrides an attribute of the same name on instrument {instrument}" + ) instrument.parameters[name] = self diff --git a/tests/parameter/test_parameter_override.py b/tests/parameter/test_parameter_override.py new file mode 100644 index 00000000000..90bad80030b --- /dev/null +++ b/tests/parameter/test_parameter_override.py @@ -0,0 +1,99 @@ +import pytest + +from qcodes.instrument import Instrument + + +class DummyOverrideInstr(Instrument): + def __init__(self, name, **kwargs): + """ + This instrument errors because it tries to override an attribute with a parameter. + Removing the parameter using `self.parameters.pop("voltage")` is not safe but + would work if the parameter was not assigned to an attribute. + """ + super().__init__(name, **kwargs) + self.voltage = self.add_parameter("voltage", set_cmd=None, get_cmd=None) + + self.parameters.pop("voltage") + + self.voltage = self.add_parameter("voltage", set_cmd=None, get_cmd=None) + + +class DummyParameterIsAttrInstr(Instrument): + def __init__(self, name, **kwargs): + """ + This instrument errors because it tries to override an attribute with a parameter. + """ + super().__init__(name, **kwargs) + self.voltage = self.add_parameter("voltage", set_cmd=None, get_cmd=None) + + def voltage(self): + return 0 + + +class DummyParameterIsPropertyInstr(Instrument): + def __init__(self, name, **kwargs): + """ + We allow overriding a property since this pattern has been seen in the wild + to define an interface for the instrument. + """ + super().__init__(name, **kwargs) + self.add_parameter("voltage", set_cmd=None, get_cmd=None) # type: ignore + + @property + def voltage(self): + return self.parameters["voltage"] + + +class DummyInstr(Instrument): + def __init__(self, name, **kwargs): + """ + We allow overriding a property since this pattern has been seen in the wild + to define an interface for the instrument. + """ + super().__init__(name, **kwargs) + self.voltage = self.add_parameter("voltage", set_cmd=None, get_cmd=None) + self.add_parameter("current", set_cmd=None, get_cmd=None) + self.some_attr = 1 + + +def test_overriding_parameter_attribute_with_parameter_raises(): + with pytest.raises( + KeyError, + match="Parameter voltage overrides an attribute of the same name on instrument", + ): + DummyOverrideInstr("my_instr") + + +def test_overriding_attribute_with_parameter_raises(): + with pytest.raises( + KeyError, + match="Parameter voltage overrides an attribute of the same name on instrument", + ): + DummyParameterIsAttrInstr("my_instr") + + +def test_overriding_property_with_parameter_works(request): + request.addfinalizer(DummyParameterIsPropertyInstr.close_all) + DummyParameterIsPropertyInstr("my_instr") + + +def test_removing_parameter_works(request): + request.addfinalizer(DummyInstr.close_all) + a = DummyInstr("my_instr") + + a.remove_parameter("voltage") + + a.remove_parameter("current") + + with pytest.raises(KeyError, match="some_attr"): + a.remove_parameter("some_attr") + + assert a.some_attr == 1 + + +def test_removed_parameter_from_prop_instrument_works(request): + request.addfinalizer(DummyParameterIsPropertyInstr.close_all) + a = DummyParameterIsPropertyInstr("my_instr") + a.remove_parameter("voltage") + a.add_parameter("voltage", set_cmd=None, get_cmd=None) + a.voltage.set(1)