From dab3aeef905e1fe04ba3d8b38db016314ffb7fdf Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Wed, 12 Jun 2024 12:39:48 +0200 Subject: [PATCH 1/6] Error on setting parameter if this already exists as an attribute but not a property --- src/qcodes/parameters/parameter_base.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/qcodes/parameters/parameter_base.py b/src/qcodes/parameters/parameter_base.py index 1cb10435718..313d1ee273e 100644 --- a/src/qcodes/parameters/parameter_base.py +++ b/src/qcodes/parameters/parameter_base.py @@ -316,10 +316,18 @@ 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 existing_parameter: - if not existing_parameter.abstract: + if found_as_delegate or found_as_attr: + existing_parameter = instrument.parameters.get(name, None) + if existing_parameter is not None and not existing_parameter.abstract: raise KeyError( f"Duplicate parameter name {name} on instrument {instrument}" ) From a54aaf43fc871a24ef6e7154200cdc7da220f82d Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Thu, 27 Jun 2024 11:01:19 +0200 Subject: [PATCH 2/6] Also error if a parameter overrides an attribute --- src/qcodes/parameters/parameter_base.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qcodes/parameters/parameter_base.py b/src/qcodes/parameters/parameter_base.py index 313d1ee273e..6a0ee182d41 100644 --- a/src/qcodes/parameters/parameter_base.py +++ b/src/qcodes/parameters/parameter_base.py @@ -327,10 +327,17 @@ def __init__( if found_as_delegate or found_as_attr: existing_parameter = instrument.parameters.get(name, None) + 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 From df79e0834ca4ae559bfb043a9c2a01ffbe1e703c Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Thu, 27 Jun 2024 11:07:19 +0200 Subject: [PATCH 3/6] Add tests for parameter override --- tests/parameter/test_parameter_override.py | 74 ++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 tests/parameter/test_parameter_override.py diff --git a/tests/parameter/test_parameter_override.py b/tests/parameter/test_parameter_override.py new file mode 100644 index 00000000000..e67ccb10b0c --- /dev/null +++ b/tests/parameter/test_parameter_override.py @@ -0,0 +1,74 @@ +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.voltage = 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) + + +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(): + DummyParameterIsPropertyInstr("my_instr") From 77b1794d4f088510f8f28710674a53a978612cb4 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Thu, 27 Jun 2024 11:20:40 +0200 Subject: [PATCH 4/6] Add method to remove parameter safely --- src/qcodes/instrument/instrument_base.py | 21 ++++++++++++++++++++ tests/parameter/test_parameter_override.py | 23 ++++++++++++++++++++-- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/qcodes/instrument/instrument_base.py b/src/qcodes/instrument/instrument_base.py index fe7100747d7..1a76d1aa238 100644 --- a/src/qcodes/instrument/instrument_base.py +++ b/src/qcodes/instrument/instrument_base.py @@ -186,6 +186,27 @@ 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. + + 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/tests/parameter/test_parameter_override.py b/tests/parameter/test_parameter_override.py index e67ccb10b0c..a03b1e6dc7e 100644 --- a/tests/parameter/test_parameter_override.py +++ b/tests/parameter/test_parameter_override.py @@ -37,7 +37,7 @@ def __init__(self, name, **kwargs): to define an interface for the instrument. """ super().__init__(name, **kwargs) - self.voltage = self.add_parameter("voltage", set_cmd=None, get_cmd=None) # type: ignore + self.add_parameter("voltage", set_cmd=None, get_cmd=None) # type: ignore @property def voltage(self): @@ -52,6 +52,7 @@ def __init__(self, name, **kwargs): """ 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) def test_overriding_parameter_attribute_with_parameter_raises(): @@ -70,5 +71,23 @@ def test_overriding_attribute_with_parameter_raises(): DummyParameterIsAttrInstr("my_instr") -def test_overriding_property_with_parameter_works(): +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") + + +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) From 49b0d9ed55af9f9de200be15d68b06060e52fff1 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Thu, 27 Jun 2024 11:26:11 +0200 Subject: [PATCH 5/6] Also test handling of none parameter attr --- src/qcodes/instrument/instrument_base.py | 4 +++- tests/parameter/test_parameter_override.py | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/qcodes/instrument/instrument_base.py b/src/qcodes/instrument/instrument_base.py index 1a76d1aa238..519b1ce66f9 100644 --- a/src/qcodes/instrument/instrument_base.py +++ b/src/qcodes/instrument/instrument_base.py @@ -194,11 +194,13 @@ def remove_parameter(self, name: str) -> None: 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 + Raises: + KeyError: If the parameter does not exist on the instrument. """ self.parameters.pop(name) diff --git a/tests/parameter/test_parameter_override.py b/tests/parameter/test_parameter_override.py index a03b1e6dc7e..90bad80030b 100644 --- a/tests/parameter/test_parameter_override.py +++ b/tests/parameter/test_parameter_override.py @@ -53,6 +53,7 @@ def __init__(self, name, **kwargs): 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(): @@ -84,6 +85,11 @@ def test_removing_parameter_works(request): 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) From 81465a18078b18c00ab89b58943a8072beaa6370 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Thu, 27 Jun 2024 13:12:50 +0200 Subject: [PATCH 6/6] Add changelog for 6174 --- docs/changes/0.46.0.rst | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) 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: ---------