From c5b585a229a8a2d82aa1c5022933bf40d292dd6a Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 07:58:39 +0100 Subject: [PATCH 01/11] Split AMI430 3D component and vector getters The getters returned a float for one component and a list for three, so none of the field parameters could be annotated. Single components are now typed float and the read-only vectors list[float]. --- .../american_magnetics/AMI430_visa.py | 134 ++++++++---------- tests/drivers/test_ami430_visa.py | 35 +++++ 2 files changed, 96 insertions(+), 73 deletions(-) diff --git a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py index 64d490b9eb7d..425283a2d69f 100644 --- a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py +++ b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py @@ -729,59 +729,67 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) # Get-only parameters that return a measured value - self.cartesian_measured: Parameter = self.add_parameter( + self.cartesian_measured: Parameter[list[float], Self] = self.add_parameter( "cartesian_measured", get_cmd=partial(self._get_measured, "x", "y", "z"), unit="T", ) """Parameter cartesian_measured""" - self.x_measured: Parameter = self.add_parameter( - "x_measured", get_cmd=partial(self._get_measured, "x"), unit="T" + self.x_measured: Parameter[float, Self] = self.add_parameter( + "x_measured", get_cmd=partial(self._get_measured_component, "x"), unit="T" ) """Parameter x_measured""" - self.y_measured: Parameter = self.add_parameter( - "y_measured", get_cmd=partial(self._get_measured, "y"), unit="T" + self.y_measured: Parameter[float, Self] = self.add_parameter( + "y_measured", get_cmd=partial(self._get_measured_component, "y"), unit="T" ) """Parameter y_measured""" - self.z_measured: Parameter = self.add_parameter( - "z_measured", get_cmd=partial(self._get_measured, "z"), unit="T" + self.z_measured: Parameter[float, Self] = self.add_parameter( + "z_measured", get_cmd=partial(self._get_measured_component, "z"), unit="T" ) """Parameter z_measured""" - self.spherical_measured: Parameter = self.add_parameter( + self.spherical_measured: Parameter[list[float], Self] = self.add_parameter( "spherical_measured", get_cmd=partial(self._get_measured, "r", "theta", "phi"), unit="T", ) """Parameter spherical_measured""" - self.phi_measured: Parameter = self.add_parameter( - "phi_measured", get_cmd=partial(self._get_measured, "phi"), unit="deg" + self.phi_measured: Parameter[float, Self] = self.add_parameter( + "phi_measured", + get_cmd=partial(self._get_measured_component, "phi"), + unit="deg", ) """Parameter phi_measured""" - self.theta_measured: Parameter = self.add_parameter( - "theta_measured", get_cmd=partial(self._get_measured, "theta"), unit="deg" + self.theta_measured: Parameter[float, Self] = self.add_parameter( + "theta_measured", + get_cmd=partial(self._get_measured_component, "theta"), + unit="deg", ) """Parameter theta_measured""" - self.field_measured: Parameter = self.add_parameter( - "field_measured", get_cmd=partial(self._get_measured, "r"), unit="T" + self.field_measured: Parameter[float, Self] = self.add_parameter( + "field_measured", + get_cmd=partial(self._get_measured_component, "r"), + unit="T", ) """Parameter field_measured""" - self.cylindrical_measured: Parameter = self.add_parameter( + self.cylindrical_measured: Parameter[list[float], Self] = self.add_parameter( "cylindrical_measured", get_cmd=partial(self._get_measured, "rho", "phi", "z"), unit="T", ) """Parameter cylindrical_measured""" - self.rho_measured: Parameter = self.add_parameter( - "rho_measured", get_cmd=partial(self._get_measured, "rho"), unit="T" + self.rho_measured: Parameter[float, Self] = self.add_parameter( + "rho_measured", + get_cmd=partial(self._get_measured_component, "rho"), + unit="T", ) """Parameter rho_measured""" @@ -795,28 +803,28 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) """Parameter cartesian""" - self.x: Parameter = self.add_parameter( + self.x: Parameter[float, Self] = self.add_parameter( "x", - get_cmd=partial(self._get_setpoints, ("x",)), - set_cmd=partial(self._set_setpoints, ("x",)), + get_cmd=partial(self._get_setpoint, "x"), + set_cmd=partial(self._set_setpoint, "x"), unit="T", vals=Numbers(), ) """Parameter x""" - self.y: Parameter = self.add_parameter( + self.y: Parameter[float, Self] = self.add_parameter( "y", - get_cmd=partial(self._get_setpoints, ("y",)), - set_cmd=partial(self._set_setpoints, ("y",)), + get_cmd=partial(self._get_setpoint, "y"), + set_cmd=partial(self._set_setpoint, "y"), unit="T", vals=Numbers(), ) """Parameter y""" - self.z: Parameter = self.add_parameter( + self.z: Parameter[float, Self] = self.add_parameter( "z", - get_cmd=partial(self._get_setpoints, ("z",)), - set_cmd=partial(self._set_setpoints, ("z",)), + get_cmd=partial(self._get_setpoint, "z"), + set_cmd=partial(self._set_setpoint, "z"), unit="T", vals=Numbers(), ) @@ -831,28 +839,28 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) """Parameter spherical""" - self.phi: Parameter = self.add_parameter( + self.phi: Parameter[float, Self] = self.add_parameter( "phi", - get_cmd=partial(self._get_setpoints, ("phi",)), - set_cmd=partial(self._set_setpoints, ("phi",)), + get_cmd=partial(self._get_setpoint, "phi"), + set_cmd=partial(self._set_setpoint, "phi"), unit="deg", vals=Numbers(), ) """Parameter phi""" - self.theta: Parameter = self.add_parameter( + self.theta: Parameter[float, Self] = self.add_parameter( "theta", - get_cmd=partial(self._get_setpoints, ("theta",)), - set_cmd=partial(self._set_setpoints, ("theta",)), + get_cmd=partial(self._get_setpoint, "theta"), + set_cmd=partial(self._set_setpoint, "theta"), unit="deg", vals=Numbers(), ) """Parameter theta""" - self.field: Parameter = self.add_parameter( + self.field: Parameter[float, Self] = self.add_parameter( "field", - get_cmd=partial(self._get_setpoints, ("r",)), - set_cmd=partial(self._set_setpoints, ("r",)), + get_cmd=partial(self._get_setpoint, "r"), + set_cmd=partial(self._set_setpoint, "r"), unit="T", vals=Numbers(), ) @@ -867,10 +875,10 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) """Parameter cylindrical""" - self.rho: Parameter = self.add_parameter( + self.rho: Parameter[float, Self] = self.add_parameter( "rho", - get_cmd=partial(self._get_setpoints, ("rho",)), - set_cmd=partial(self._set_setpoints, ("rho",)), + get_cmd=partial(self._get_setpoint, "rho"), + set_cmd=partial(self._set_setpoint, "rho"), unit="T", vals=Numbers(), ) @@ -1247,11 +1255,11 @@ def _request_field_change(self, instrument: AMIModel430, value: NumberType) -> N performed by this 3D driver. """ if instrument is self._instrument_x: - self._set_setpoints(("x",), (float(value),)) + self._set_setpoint("x", float(value)) elif instrument is self._instrument_y: - self._set_setpoints(("y",), (float(value),)) + self._set_setpoint("y", float(value)) elif instrument is self._instrument_z: - self._set_setpoints(("z",), (float(value),)) + self._set_setpoint("z", float(value)) else: msg = "This magnet doesnt belong to its specified parent {}" raise NameError(msg.format(self)) @@ -1263,42 +1271,22 @@ def _get_measured_field_vector(self) -> FieldVector: z=self._instrument_z.field(), ) - def _get_measured(self, *names: str) -> float | list[float]: - measured_field_vector = self._get_measured_field_vector() - - measured_values = measured_field_vector.get_components(*names) + def _get_measured_component(self, name: str) -> float: + return self._get_measured_field_vector().get_components(name)[0] - # Convert angles from radians to degrees - d = dict(zip(names, measured_values)) + def _get_measured(self, *names: str) -> list[float]: + # get_components returns the values in the order of the names + return self._get_measured_field_vector().get_components(*names) - # Do not do "return list(d.values())", because then there is - # no guaranty that the order in which the values are returned - # is the same as the original intention - value_list = [d[name] for name in names] + def _get_setpoint(self, name: str) -> float: + return self._set_point.get_components(name)[0] - if len(names) == 1: - return_value: list[float] | float = value_list[0] - else: - return_value = value_list - - return return_value - - def _get_setpoints(self, names: Sequence[str]) -> float | list[float]: - measured_values = self._set_point.get_components(*names) - - # Convert angles from radians to degrees - d = dict(zip(names, measured_values)) - value_list = [d[name] for name in names] - # Do not do "return list(d.values())", because then there is - # no guarantee that the order in which the values are returned - # is the same as the original intention - - if len(names) == 1: - return_value: list[float] | float = value_list[0] - else: - return_value = value_list + def _get_setpoints(self, names: Sequence[str]) -> list[float]: + # get_components returns the values in the order of the names + return self._set_point.get_components(*names) - return return_value + def _set_setpoint(self, name: str, value: float) -> None: + self._set_setpoints((name,), (value,)) def _set_setpoints(self, names: Sequence[str], values: Sequence[float]) -> None: kwargs = dict(zip(names, np.atleast_1d(values))) diff --git a/tests/drivers/test_ami430_visa.py b/tests/drivers/test_ami430_visa.py index 3bb380f1458e..bf337402e87f 100644 --- a/tests/drivers/test_ami430_visa.py +++ b/tests/drivers/test_ami430_visa.py @@ -630,6 +630,41 @@ def test_measured( assert np.allclose(cylindrical, [cylindrical_rho, spherical_phi, cartesian_z]) +_SINGLE_COMPONENTS = ("x", "y", "z", "field", "theta", "phi", "rho") +_VECTORS = ("cartesian", "spherical", "cylindrical") + + +def test_component_and_vector_types(current_driver: AMIModel4303D) -> None: + # single components are plain floats while the coordinate vectors are + # lists of three floats, both for the set points and the measured values + current_driver.cartesian((0.1, 0.2, 0.3)) + + for name in _SINGLE_COMPONENTS: + for parameter_name in (name, f"{name}_measured"): + value = current_driver.parameters[parameter_name]() + assert isinstance(value, float), parameter_name + + for name in _VECTORS: + for parameter_name in (name, f"{name}_measured"): + value = current_driver.parameters[parameter_name]() + assert isinstance(value, list), parameter_name + assert len(value) == 3 + assert all(isinstance(component, float) for component in value) + + +def test_set_single_component(current_driver: AMIModel4303D) -> None: + current_driver.cartesian((0.1, 0.2, 0.3)) + + current_driver.x(0.4) + current_driver.field(0.5) + + assert np.isclose(current_driver.field(), 0.5) + assert np.isclose(current_driver.field_measured(), 0.5) + # setting the field magnitude keeps the direction of (0.4, 0.2, 0.3) + direction = np.array([0.4, 0.2, 0.3]) / np.linalg.norm([0.4, 0.2, 0.3]) + assert np.allclose(current_driver.cartesian(), 0.5 * direction) + + def get_ramp_down_order(messages: list[str]) -> list[str]: order = [] From f4556daab408786f4feafab8b50717514c69b13c Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 07:59:19 +0100 Subject: [PATCH 02/11] Remove placeholder unit from AMI430 vector set points spherical and cylindrical had unit "tuple?". Their components have mixed units, which are now described in the docstrings. --- .../american_magnetics/AMI430_visa.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py index 425283a2d69f..744a9982a4e4 100644 --- a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py +++ b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py @@ -834,10 +834,14 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: "spherical", get_cmd=partial(self._get_setpoints, ("r", "theta", "phi")), set_cmd=partial(self._set_setpoints, ("r", "theta", "phi")), - unit="tuple?", vals=Anything(), + docstring="Set point in spherical coordinates (field, theta, phi), " + "with the field in T and the angles in degrees.", ) - """Parameter spherical""" + """ + Set point in spherical coordinates (field, theta, phi), with the field in T + and the angles in degrees. + """ self.phi: Parameter[float, Self] = self.add_parameter( "phi", @@ -870,10 +874,14 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: "cylindrical", get_cmd=partial(self._get_setpoints, ("rho", "phi", "z")), set_cmd=partial(self._set_setpoints, ("rho", "phi", "z")), - unit="tuple?", vals=Anything(), + docstring="Set point in cylindrical coordinates (rho, phi, z), " + "with rho and z in T and phi in degrees.", ) - """Parameter cylindrical""" + """ + Set point in cylindrical coordinates (rho, phi, z), with rho and z in T and + phi in degrees. + """ self.rho: Parameter[float, Self] = self.add_parameter( "rho", From 33555c9519df9eee4c7d303686246f0fb800b578 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 08:10:39 +0100 Subject: [PATCH 03/11] Annotate remaining AMI430 3D parameters Create ramp_mode, ramping_state_check_interval and vector_ramp_rate with add_parameter like the other parameters, so they can be typed. vector_ramp_rate is float | None since it is unset until set. --- .../american_magnetics/AMI430_visa.py | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py index 744a9982a4e4..f61faf562180 100644 --- a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py +++ b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py @@ -20,7 +20,6 @@ ) from qcodes.math_utils import FieldVector from qcodes.metadatable import normalize_snapshot_update -from qcodes.parameters import Parameter from qcodes.utils.types import NumberType from qcodes.validators import Anything, Bool, Enum, Ints, Numbers @@ -28,6 +27,7 @@ from typing import Unpack from qcodes.metadatable import SnapshotUpdate + from qcodes.parameters import Parameter log = logging.getLogger(__name__) @@ -897,28 +897,28 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) """Parameter block_during_ramp""" - self.ramp_mode = Parameter( - name="ramp_mode", - instrument=self, + self.ramp_mode: Parameter[str, Self] = self.add_parameter( + "ramp_mode", get_cmd=None, set_cmd=None, vals=Enum("default", "simultaneous"), initial_value="default", ) + """Parameter ramp_mode""" - self.ramping_state_check_interval = Parameter( - name="ramping_state_check_interval", - instrument=self, + self.ramping_state_check_interval: Parameter[float, Self] = self.add_parameter( + "ramping_state_check_interval", initial_value=0.05, unit="s", vals=Numbers(0, 10), set_cmd=None, get_cmd=None, ) + """Parameter ramping_state_check_interval""" - self.vector_ramp_rate = Parameter( - name="vector_ramp_rate", - instrument=self, + # None until set, which is checked before a simultaneous ramp + self.vector_ramp_rate: Parameter[float | None, Self] = self.add_parameter( + "vector_ramp_rate", unit="T/s", vals=Numbers(min_value=0.0), set_cmd=None, @@ -1120,7 +1120,8 @@ def _adjust_child_instruments(self, values: tuple[float, float, float]) -> None: def _update_individual_axes_ramp_rates( self, values: tuple[float, float, float] ) -> None: - if self.vector_ramp_rate() is None or self.vector_ramp_rate() == 0: + vector_ramp_rate = self.vector_ramp_rate() + if vector_ramp_rate is None or vector_ramp_rate == 0: raise ValueError( "The value of the `vector_ramp_rate` Parameter is " "currently None or 0. Set it to an appropriate " @@ -1130,7 +1131,7 @@ def _update_individual_axes_ramp_rates( new_axes_ramp_rates = self.calculate_axes_ramp_rates_from_vector_ramp_rate( start=self._get_measured_field_vector(), setpoint=FieldVector(x=values[0], y=values[1], z=values[2]), - vector_ramp_rate=self.vector_ramp_rate.get(), + vector_ramp_rate=vector_ramp_rate, ) instruments = (self._instrument_x, self._instrument_y, self._instrument_z) for instrument, new_axis_ramp_rate in zip(instruments, new_axes_ramp_rates): From 67d89a2b18231756e0920289a39243166dab9ba0 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 08:15:53 +0100 Subject: [PATCH 04/11] Use magnet field units for AMI430 3D field parameters The 3D field parameters are read from and set on the individual magnets in their field units, which can be tesla or kilogauss, but were always labelled T. Follow the magnets' units, and warn if they differ. --- .../american_magnetics/AMI430_visa.py | 62 ++++++++++++++++--- tests/drivers/test_ami430_visa.py | 62 +++++++++++++++++++ 2 files changed, 117 insertions(+), 7 deletions(-) diff --git a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py index f61faf562180..5009122b9900 100644 --- a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py +++ b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py @@ -583,6 +583,10 @@ def _update_units( self.current_ramp_limit.unit = f"A/{ramp_rate_units_short}" self.field_ramp_limit.unit = f"{field_units_short}/{ramp_rate_units_short}" + # the field parameters of a 3D driver are in the units of the magnets + if self._parent_instrument is not None: + self._parent_instrument._update_field_units() + # And update scaling factors # Note: we don't update field_ramp_limit scale as it redirects # to ramp_rate_limit; we don't update ramp_rate units as @@ -663,7 +667,8 @@ def __init__( instance for controlling the Z axis of magnetic field field_limit: a number for maximum allows magnetic field or an iterable of callable field limit functions that define - region(s) of allowed values in 3D magnetic field space + region(s) of allowed values in 3D magnetic field space, in + the field units of the magnets (tesla or kilogauss) **kwargs: kwargs are forwarded to base class. """ @@ -836,11 +841,12 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: set_cmd=partial(self._set_setpoints, ("r", "theta", "phi")), vals=Anything(), docstring="Set point in spherical coordinates (field, theta, phi), " - "with the field in T and the angles in degrees.", + "with the field in the field units of the magnets and the angles " + "in degrees.", ) """ - Set point in spherical coordinates (field, theta, phi), with the field in T - and the angles in degrees. + Set point in spherical coordinates (field, theta, phi), with the field in + the field units of the magnets and the angles in degrees. """ self.phi: Parameter[float, Self] = self.add_parameter( @@ -876,11 +882,12 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: set_cmd=partial(self._set_setpoints, ("rho", "phi", "z")), vals=Anything(), docstring="Set point in cylindrical coordinates (rho, phi, z), " - "with rho and z in T and phi in degrees.", + "with rho and z in the field units of the magnets and phi in " + "degrees.", ) """ - Set point in cylindrical coordinates (rho, phi, z), with rho and z in T and - phi in degrees. + Set point in cylindrical coordinates (rho, phi, z), with rho and z in the + field units of the magnets and phi in degrees. """ self.rho: Parameter[float, Self] = self.add_parameter( @@ -929,12 +936,53 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) """Ramp rate along a line (vector) in 3D field space""" + # the field parameters are in the field units of the magnets + self._update_field_units() + self._exit_stack = ExitStack() def get_idn(self) -> dict[str, str | None]: idparts = ["American Magnetics", self.name, None, None] return dict(zip(("vendor", "model", "serial", "firmware"), idparts)) + def _update_field_units(self) -> None: + """ + Use the field units of the magnets (tesla or kilogauss) as the unit of + the field parameters, since their values are in the units of the + magnets. This is called whenever the field units of a magnet change. + """ + instruments = (self._instrument_x, self._instrument_y, self._instrument_z) + units = {instrument.field.unit for instrument in instruments} + if len(units) == 1: + (unit,) = units + else: + unit = "" + self.log.warning( + "The magnets use different field units (%s), so the field " + "values of %s combine different units.", + ", ".join( + f"{instrument.short_name}: {instrument.field.unit}" + for instrument in instruments + ), + self.name, + ) + field_parameters: tuple[Parameter, ...] = ( + self.cartesian, + self.x, + self.y, + self.z, + self.field, + self.rho, + self.cartesian_measured, + self.x_measured, + self.y_measured, + self.z_measured, + self.field_measured, + self.rho_measured, + ) + for parameter in field_parameters: + parameter.unit = unit + def _set_vector_ramp_rate_units(self, val: float) -> float: _, common_ramp_rate_units = self._raise_if_not_same_field_and_ramp_rate_units() self.vector_ramp_rate.unit = common_ramp_rate_units diff --git a/tests/drivers/test_ami430_visa.py b/tests/drivers/test_ami430_visa.py index bf337402e87f..9b2a86153568 100644 --- a/tests/drivers/test_ami430_visa.py +++ b/tests/drivers/test_ami430_visa.py @@ -665,6 +665,68 @@ def test_set_single_component(current_driver: AMIModel4303D) -> None: assert np.allclose(current_driver.cartesian(), 0.5 * direction) +def _field_units(driver: AMIModel4303D) -> set[str]: + field_parameters = ( + driver.cartesian, + driver.x, + driver.y, + driver.z, + driver.field, + driver.rho, + driver.cartesian_measured, + driver.x_measured, + driver.y_measured, + driver.z_measured, + driver.field_measured, + driver.rho_measured, + ) + return {parameter.unit for parameter in field_parameters} + + +def test_field_units_follow_magnets( + current_driver: AMIModel4303D, + magnet_axes_instances: tuple[AMIModel430, AMIModel430, AMIModel430], +) -> None: + # the field values are in the field units of the magnets, which can be + # tesla or kilogauss + assert _field_units(current_driver) == {"T"} + + for magnet in magnet_axes_instances: + magnet.field_units("kilogauss") + + assert _field_units(current_driver) == {"kG"} + # the angles are not affected + assert current_driver.phi.unit == "deg" + assert current_driver.theta_measured.unit == "deg" + + +def test_mixed_field_units_warn( + current_driver: AMIModel4303D, + magnet_axes_instances: tuple[AMIModel430, AMIModel430, AMIModel430], + caplog: LogCaptureFixture, +) -> None: + mag_x, _, _ = magnet_axes_instances + + with caplog.at_level(logging.WARNING): + mag_x.field_units("kilogauss") + + assert _field_units(current_driver) == {""} + assert "The magnets use different field units (x: kG, y: T, z: T)" in caplog.text + + +def test_field_units_at_init_follow_magnets( + magnet_axes_instances: tuple[AMIModel430, AMIModel430, AMIModel430], +) -> None: + for magnet in magnet_axes_instances: + magnet.field_units("kilogauss") + + driver = AMIModel4303D("AMI430_3D", *magnet_axes_instances, field_limit) + try: + assert _field_units(driver) == {"kG"} + finally: + driver.close() + + def get_ramp_down_order(messages: list[str]) -> list[str]: order = [] From adf024ef0bd6f07652d9875aff74e3b091e30f13 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 08:15:58 +0100 Subject: [PATCH 05/11] Add newsfragment for AMI430 unit fixes --- docs/changes/newsfragments/xxxx.improved_driver | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changes/newsfragments/xxxx.improved_driver diff --git a/docs/changes/newsfragments/xxxx.improved_driver b/docs/changes/newsfragments/xxxx.improved_driver new file mode 100644 index 000000000000..7763112c71f9 --- /dev/null +++ b/docs/changes/newsfragments/xxxx.improved_driver @@ -0,0 +1,5 @@ +The field parameters of ``AMIModel4303D``, such as ``cartesian``, ``x`` and ``field_measured``, +now use the field units of the individual magnets, tesla or kilogauss, instead of always +reporting tesla. A warning is logged if the magnets use different field units. Also removed the +placeholder unit ``tuple?`` of ``AMIModel4303D.spherical`` and ``cylindrical``, whose components +have different units, which are now described in their docstrings. From 2921adcf4951148813ea5734d16e650607fc2aaa Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 08:23:54 +0100 Subject: [PATCH 06/11] Rename newsfragments to PR 8679 --- .../newsfragments/{xxxx.improved_driver => 8679.improved_driver} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename docs/changes/newsfragments/{xxxx.improved_driver => 8679.improved_driver} (100%) diff --git a/docs/changes/newsfragments/xxxx.improved_driver b/docs/changes/newsfragments/8679.improved_driver similarity index 100% rename from docs/changes/newsfragments/xxxx.improved_driver rename to docs/changes/newsfragments/8679.improved_driver From 0940eadb79b6ce24b1c54af024367024d489f320 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 21:40:48 +0100 Subject: [PATCH 07/11] Detach AMI430 magnets when closing the 3D driver The magnets kept a reference to the closed 3D driver, so set_field, zero and changing units on a magnet that was still open failed. --- .../american_magnetics/AMI430_visa.py | 13 +++++++++++++ tests/drivers/test_ami430_visa.py | 18 ++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py index 5009122b9900..d2433be5c798 100644 --- a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py +++ b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py @@ -945,6 +945,19 @@ def get_idn(self) -> dict[str, str | None]: idparts = ["American Magnetics", self.name, None, None] return dict(zip(("vendor", "model", "serial", "firmware"), idparts)) + def close(self) -> None: + # the magnets can stay open after this driver is closed, so they must + # no longer delegate to it. A magnet that was closed first no longer + # has the attribute. + for name in ("_instrument_x", "_instrument_y", "_instrument_z"): + instrument: AMIModel430 | None = getattr(self, name, None) + if ( + instrument is not None + and getattr(instrument, "_parent_instrument", None) is self + ): + instrument._parent_instrument = None + super().close() + def _update_field_units(self) -> None: """ Use the field units of the magnets (tesla or kilogauss) as the unit of diff --git a/tests/drivers/test_ami430_visa.py b/tests/drivers/test_ami430_visa.py index 9b2a86153568..d0b09e73e028 100644 --- a/tests/drivers/test_ami430_visa.py +++ b/tests/drivers/test_ami430_visa.py @@ -727,6 +727,24 @@ def test_field_units_at_init_follow_magnets( driver.close() +def test_magnets_work_after_3d_driver_is_closed( + magnet_axes_instances: tuple[AMIModel430, AMIModel430, AMIModel430], +) -> None: + mag_x, _, _ = magnet_axes_instances + driver = AMIModel4303D("AMI430_3D", *magnet_axes_instances, field_limit) + assert mag_x._parent_instrument is driver + + driver.close() + + # the magnets no longer delegate to the closed 3D driver + assert mag_x._parent_instrument is None + mag_x.field_units("kilogauss") + assert mag_x.field.unit == "kG" + mag_x.set_field(0.1) + assert mag_x.field() == pytest.approx(0.1) + mag_x.zero() + + def get_ramp_down_order(messages: list[str]) -> list[str]: order = [] From cc6bbfe7a5b9c15cb5e5bc853494140e445553cd Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 21:42:17 +0100 Subject: [PATCH 08/11] Block AMI430 field unit changes in a 3D driver The 3D driver's set point and field limit are in the field units of the magnets, so changing them between tesla and kilogauss silently changed their meaning by a factor of 10. Raise instead, and take the units of the 3D field parameters from the magnets only at creation. --- docs/changes/newsfragments/8679.breaking | 5 ++ .../newsfragments/8679.improved_driver | 4 +- .../american_magnetics/AMI430_visa.py | 38 +++++++++-- tests/drivers/test_ami430_visa.py | 66 +++++++++++++------ 4 files changed, 85 insertions(+), 28 deletions(-) create mode 100644 docs/changes/newsfragments/8679.breaking diff --git a/docs/changes/newsfragments/8679.breaking b/docs/changes/newsfragments/8679.breaking new file mode 100644 index 000000000000..36aab8e965a3 --- /dev/null +++ b/docs/changes/newsfragments/8679.breaking @@ -0,0 +1,5 @@ +The field units of an ``AMIModel430`` can no longer be changed while it is part of an +``AMIModel4303D``, and doing so now raises a ``RuntimeError``. The set point and field limit of +the 3D driver are in the field units of the magnets, so changing them silently changed their +meaning by a factor of 10. Set the field units before creating the 3D driver, or close the 3D +driver first. diff --git a/docs/changes/newsfragments/8679.improved_driver b/docs/changes/newsfragments/8679.improved_driver index 7763112c71f9..34a92b5306cb 100644 --- a/docs/changes/newsfragments/8679.improved_driver +++ b/docs/changes/newsfragments/8679.improved_driver @@ -2,4 +2,6 @@ The field parameters of ``AMIModel4303D``, such as ``cartesian``, ``x`` and ``fi now use the field units of the individual magnets, tesla or kilogauss, instead of always reporting tesla. A warning is logged if the magnets use different field units. Also removed the placeholder unit ``tuple?`` of ``AMIModel4303D.spherical`` and ``cylindrical``, whose components -have different units, which are now described in their docstrings. +have different units, which are now described in their docstrings. Closing an +``AMIModel4303D`` now detaches its magnets, so ``set_field``, ``zero`` and changing the units of +a magnet that is still open no longer fail. diff --git a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py index d2433be5c798..909ac126a5a5 100644 --- a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py +++ b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py @@ -259,7 +259,7 @@ def __init__( self.field_units: Parameter[str, Self] = self.add_parameter( "field_units", get_cmd="FIELD:UNITS?", - set_cmd=(lambda units: self._update_units(field_units=units)), + set_cmd=self._set_field_units, val_mapping={"kilogauss": 0, "tesla": 1}, ) """Parameter field_units""" @@ -551,6 +551,31 @@ def _update_coil_constant(self, new_coil_constant: float | None = None) -> float # Return new coil constant return new_coil_constant + def _set_field_units(self, field_units: int) -> None: + """ + Set the field units, 0 for kilogauss and 1 for tesla. + + Raises: + RuntimeError: if the units would change while this magnet is part + of a 3D driver, whose set point and field limit are in the + field units of the magnets. + + """ + if self._parent_instrument is not None: + assert self.field_units.inverse_val_mapping is not None + new_units = self._SHORT_UNITS[ + self.field_units.inverse_val_mapping[field_units] + ] + if new_units != self.field.unit: + raise RuntimeError( + f"Cannot change the field units of {self.name} while it is " + f"part of the 3D driver {self._parent_instrument.name}, " + "since its set point and field limit are in the field " + "units of the magnets. Set the field units before " + "creating the 3D driver, or close the 3D driver first." + ) + self._update_units(field_units=field_units) + def _update_units( self, ramp_rate_units: int | None = None, field_units: int | None = None ) -> None: @@ -583,10 +608,6 @@ def _update_units( self.current_ramp_limit.unit = f"A/{ramp_rate_units_short}" self.field_ramp_limit.unit = f"{field_units_short}/{ramp_rate_units_short}" - # the field parameters of a 3D driver are in the units of the magnets - if self._parent_instrument is not None: - self._parent_instrument._update_field_units() - # And update scaling factors # Note: we don't update field_ramp_limit scale as it redirects # to ramp_rate_limit; we don't update ramp_rate units as @@ -668,7 +689,9 @@ def __init__( field_limit: a number for maximum allows magnetic field or an iterable of callable field limit functions that define region(s) of allowed values in 3D magnetic field space, in - the field units of the magnets (tesla or kilogauss) + the field units of the magnets (tesla or kilogauss). The + field units of the magnets cannot be changed while they are + part of this driver. **kwargs: kwargs are forwarded to base class. """ @@ -962,7 +985,8 @@ def _update_field_units(self) -> None: """ Use the field units of the magnets (tesla or kilogauss) as the unit of the field parameters, since their values are in the units of the - magnets. This is called whenever the field units of a magnet change. + magnets. The field units of the magnets cannot be changed while they + are part of this driver. """ instruments = (self._instrument_x, self._instrument_y, self._instrument_z) units = {instrument.field.unit for instrument in instruments} diff --git a/tests/drivers/test_ami430_visa.py b/tests/drivers/test_ami430_visa.py index d0b09e73e028..f3758c42ba04 100644 --- a/tests/drivers/test_ami430_visa.py +++ b/tests/drivers/test_ami430_visa.py @@ -683,35 +683,54 @@ def _field_units(driver: AMIModel4303D) -> set[str]: return {parameter.unit for parameter in field_parameters} -def test_field_units_follow_magnets( - current_driver: AMIModel4303D, - magnet_axes_instances: tuple[AMIModel430, AMIModel430, AMIModel430], -) -> None: +def test_field_units_follow_magnets(current_driver: AMIModel4303D) -> None: # the field values are in the field units of the magnets, which can be # tesla or kilogauss assert _field_units(current_driver) == {"T"} - - for magnet in magnet_axes_instances: - magnet.field_units("kilogauss") - - assert _field_units(current_driver) == {"kG"} # the angles are not affected assert current_driver.phi.unit == "deg" assert current_driver.theta_measured.unit == "deg" -def test_mixed_field_units_warn( +def test_cannot_change_field_units_of_magnet_in_3d_driver( current_driver: AMIModel4303D, magnet_axes_instances: tuple[AMIModel430, AMIModel430, AMIModel430], - caplog: LogCaptureFixture, + mocker: MockerFixture, ) -> None: + # the set point and field limit of the 3D driver are in the field units of + # the magnets, so changing them would silently change their meaning mag_x, _, _ = magnet_axes_instances + current_driver.cartesian((0.1, 0.2, 0.3)) + write = mocker.spy(mag_x, "write") - with caplog.at_level(logging.WARNING): + with pytest.raises(RuntimeError, match="Cannot change the field units of x"): mag_x.field_units("kilogauss") - assert _field_units(current_driver) == {""} - assert "The magnets use different field units (x: kG, y: T, z: T)" in caplog.text + write.assert_not_called() + assert mag_x.field.unit == "T" + assert _field_units(current_driver) == {"T"} + assert np.allclose(current_driver.cartesian(), [0.1, 0.2, 0.3]) + + # setting the units that are already used is fine + mag_x.field_units("tesla") + + +def test_mixed_field_units_warn( + magnet_axes_instances: tuple[AMIModel430, AMIModel430, AMIModel430], + caplog: LogCaptureFixture, +) -> None: + mag_x, _, _ = magnet_axes_instances + mag_x.field_units("kilogauss") + + with caplog.at_level(logging.WARNING): + driver = AMIModel4303D("AMI430_3D", *magnet_axes_instances, field_limit) + try: + assert _field_units(driver) == {""} + assert ( + "The magnets use different field units (x: kG, y: T, z: T)" in caplog.text + ) + finally: + driver.close() def test_field_units_at_init_follow_magnets( @@ -2024,13 +2043,20 @@ def test_calculate_vector_ramp_rate_from_duration() -> None: assert np.isclose(rate, 0.5) # distance=5.0, 5.0/10.0 = 0.5 -def test_raise_if_not_same_field_units(current_driver: AMIModel4303D) -> None: +def test_raise_if_not_same_field_units( + magnet_axes_instances: tuple[AMIModel430, AMIModel430, AMIModel430], +) -> None: """Test that mismatched field_units raises ValueError.""" - current_driver._instrument_x.field_units("kilogauss") - with pytest.raises(ValueError, match="field_units"): - current_driver._raise_if_not_same_field_and_ramp_rate_units() - # Clean up - current_driver._instrument_x.field_units("tesla") + # the field units cannot change while the magnets are part of a 3D + # driver, so set up the mismatch first + mag_x, _, _ = magnet_axes_instances + mag_x.field_units("kilogauss") + driver = AMIModel4303D("AMI430_3D", *magnet_axes_instances, field_limit) + try: + with pytest.raises(ValueError, match="field_units"): + driver._raise_if_not_same_field_and_ramp_rate_units() + finally: + driver.close() def test_raise_if_not_same_ramp_rate_units(current_driver: AMIModel4303D) -> None: From dc8b9e620a2702617d907b3718b10f31031a909c Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 22:09:00 +0100 Subject: [PATCH 09/11] Annotate AMI430 vector set points cartesian, spherical and cylindrical return a list but can be set from any sequence or a numpy array, so type them as Sequence[float] | npt.NDArray. --- .../american_magnetics/AMI430_visa.py | 59 +++++++++++-------- tests/drivers/test_ami430_visa.py | 5 +- 2 files changed, 38 insertions(+), 26 deletions(-) diff --git a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py index 909ac126a5a5..d1d28d0d5681 100644 --- a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py +++ b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py @@ -26,6 +26,8 @@ if TYPE_CHECKING: from typing import Unpack + import numpy.typing as npt + from qcodes.metadatable import SnapshotUpdate from qcodes.parameters import Parameter @@ -821,13 +823,16 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) """Parameter rho_measured""" - # Get and set parameters for the set points of the coordinates - self.cartesian: Parameter = self.add_parameter( - "cartesian", - get_cmd=partial(self._get_setpoints, ("x", "y", "z")), - set_cmd=partial(self._set_setpoints, ("x", "y", "z")), - unit="T", - vals=Anything(), + # Get and set parameters for the set points of the coordinates. These + # return a list but can be set from any sequence or a numpy array. + self.cartesian: Parameter[Sequence[float] | npt.NDArray, Self] = ( + self.add_parameter( + "cartesian", + get_cmd=partial(self._get_setpoints, ("x", "y", "z")), + set_cmd=partial(self._set_setpoints, ("x", "y", "z")), + unit="T", + vals=Anything(), + ) ) """Parameter cartesian""" @@ -858,14 +863,16 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) """Parameter z""" - self.spherical: Parameter = self.add_parameter( - "spherical", - get_cmd=partial(self._get_setpoints, ("r", "theta", "phi")), - set_cmd=partial(self._set_setpoints, ("r", "theta", "phi")), - vals=Anything(), - docstring="Set point in spherical coordinates (field, theta, phi), " - "with the field in the field units of the magnets and the angles " - "in degrees.", + self.spherical: Parameter[Sequence[float] | npt.NDArray, Self] = ( + self.add_parameter( + "spherical", + get_cmd=partial(self._get_setpoints, ("r", "theta", "phi")), + set_cmd=partial(self._set_setpoints, ("r", "theta", "phi")), + vals=Anything(), + docstring="Set point in spherical coordinates (field, theta, phi), " + "with the field in the field units of the magnets and the angles " + "in degrees.", + ) ) """ Set point in spherical coordinates (field, theta, phi), with the field in @@ -899,14 +906,16 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) """Parameter field""" - self.cylindrical: Parameter = self.add_parameter( - "cylindrical", - get_cmd=partial(self._get_setpoints, ("rho", "phi", "z")), - set_cmd=partial(self._set_setpoints, ("rho", "phi", "z")), - vals=Anything(), - docstring="Set point in cylindrical coordinates (rho, phi, z), " - "with rho and z in the field units of the magnets and phi in " - "degrees.", + self.cylindrical: Parameter[Sequence[float] | npt.NDArray, Self] = ( + self.add_parameter( + "cylindrical", + get_cmd=partial(self._get_setpoints, ("rho", "phi", "z")), + set_cmd=partial(self._set_setpoints, ("rho", "phi", "z")), + vals=Anything(), + docstring="Set point in cylindrical coordinates (rho, phi, z), " + "with rho and z in the field units of the magnets and phi in " + "degrees.", + ) ) """ Set point in cylindrical coordinates (rho, phi, z), with rho and z in the @@ -1382,7 +1391,9 @@ def _get_setpoints(self, names: Sequence[str]) -> list[float]: def _set_setpoint(self, name: str, value: float) -> None: self._set_setpoints((name,), (value,)) - def _set_setpoints(self, names: Sequence[str], values: Sequence[float]) -> None: + def _set_setpoints( + self, names: Sequence[str], values: Sequence[float] | npt.NDArray + ) -> None: kwargs = dict(zip(names, np.atleast_1d(values))) set_point = FieldVector() diff --git a/tests/drivers/test_ami430_visa.py b/tests/drivers/test_ami430_visa.py index f3758c42ba04..437da389c270 100644 --- a/tests/drivers/test_ami430_visa.py +++ b/tests/drivers/test_ami430_visa.py @@ -930,7 +930,8 @@ def test_simultaneous_ramp_mode_does_not_reset_individual_axis_ramp_rates_if_non ami3d._instrument_y.ramp_rate(0.10) ami3d._instrument_z.ramp_rate(0.11) - ami3d.vector_ramp_rate(0.05) + vector_ramp_rate = 0.05 + ami3d.vector_ramp_rate(vector_ramp_rate) with caplog.at_level(logging.DEBUG, logger=LOG_NAME): # Initiate the simultaneous ramp @@ -946,7 +947,7 @@ def test_simultaneous_ramp_mode_does_not_reset_individual_axis_ramp_rates_if_non # set by the simultaneous ramp based on the vector_ramp_rate and the # setpoint magnetic field expected_ramp_rate = pytest.approx( - 0.5 / np.linalg.norm(ami3d.cartesian(), ord=2) * ami3d.vector_ramp_rate() + 0.5 / np.linalg.norm(ami3d.cartesian(), ord=2) * vector_ramp_rate ) assert ami3d._instrument_x.ramp_rate() == expected_ramp_rate assert ami3d._instrument_y.ramp_rate() == expected_ramp_rate From d1fea62539222ffbb3c25a083d4b278fb5b8cc5b Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 22:13:14 +0100 Subject: [PATCH 10/11] Return tuples from AMI430 coordinate vectors The coordinate vectors always have three components, so return a tuple and type the read-only ones as tuple[float, float, float]. The settable ones keep the Sequence[float] | NDArray type since Parameter has a single type for get and set. --- docs/changes/newsfragments/8679.breaking | 5 +++ .../american_magnetics/AMI430_visa.py | 42 +++++++++++-------- tests/drivers/test_ami430_visa.py | 4 +- 3 files changed, 32 insertions(+), 19 deletions(-) diff --git a/docs/changes/newsfragments/8679.breaking b/docs/changes/newsfragments/8679.breaking index 36aab8e965a3..9ec4ba663b63 100644 --- a/docs/changes/newsfragments/8679.breaking +++ b/docs/changes/newsfragments/8679.breaking @@ -3,3 +3,8 @@ The field units of an ``AMIModel430`` can no longer be changed while it is part the 3D driver are in the field units of the magnets, so changing them silently changed their meaning by a factor of 10. Set the field units before creating the 3D driver, or close the 3D driver first. + +The coordinate vectors of ``AMIModel4303D`` (``cartesian``, ``spherical``, ``cylindrical`` and +their ``*_measured`` versions) now return a tuple of three floats instead of a list. Code that +compares the result to a list, or modifies it, needs updating. Setting these parameters still +accepts lists, tuples and numpy arrays. diff --git a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py index d1d28d0d5681..9747067cfa07 100644 --- a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py +++ b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py @@ -759,10 +759,12 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) # Get-only parameters that return a measured value - self.cartesian_measured: Parameter[list[float], Self] = self.add_parameter( - "cartesian_measured", - get_cmd=partial(self._get_measured, "x", "y", "z"), - unit="T", + self.cartesian_measured: Parameter[tuple[float, float, float], Self] = ( + self.add_parameter( + "cartesian_measured", + get_cmd=partial(self._get_measured, ("x", "y", "z")), + unit="T", + ) ) """Parameter cartesian_measured""" @@ -781,10 +783,12 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) """Parameter z_measured""" - self.spherical_measured: Parameter[list[float], Self] = self.add_parameter( - "spherical_measured", - get_cmd=partial(self._get_measured, "r", "theta", "phi"), - unit="T", + self.spherical_measured: Parameter[tuple[float, float, float], Self] = ( + self.add_parameter( + "spherical_measured", + get_cmd=partial(self._get_measured, ("r", "theta", "phi")), + unit="T", + ) ) """Parameter spherical_measured""" @@ -809,10 +813,12 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) """Parameter field_measured""" - self.cylindrical_measured: Parameter[list[float], Self] = self.add_parameter( - "cylindrical_measured", - get_cmd=partial(self._get_measured, "rho", "phi", "z"), - unit="T", + self.cylindrical_measured: Parameter[tuple[float, float, float], Self] = ( + self.add_parameter( + "cylindrical_measured", + get_cmd=partial(self._get_measured, ("rho", "phi", "z")), + unit="T", + ) ) """Parameter cylindrical_measured""" @@ -824,7 +830,7 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: """Parameter rho_measured""" # Get and set parameters for the set points of the coordinates. These - # return a list but can be set from any sequence or a numpy array. + # return a tuple but can be set from any sequence or a numpy array. self.cartesian: Parameter[Sequence[float] | npt.NDArray, Self] = ( self.add_parameter( "cartesian", @@ -1377,16 +1383,18 @@ def _get_measured_field_vector(self) -> FieldVector: def _get_measured_component(self, name: str) -> float: return self._get_measured_field_vector().get_components(name)[0] - def _get_measured(self, *names: str) -> list[float]: + def _get_measured(self, names: tuple[str, str, str]) -> tuple[float, float, float]: # get_components returns the values in the order of the names - return self._get_measured_field_vector().get_components(*names) + a, b, c = self._get_measured_field_vector().get_components(*names) + return a, b, c def _get_setpoint(self, name: str) -> float: return self._set_point.get_components(name)[0] - def _get_setpoints(self, names: Sequence[str]) -> list[float]: + def _get_setpoints(self, names: tuple[str, str, str]) -> tuple[float, float, float]: # get_components returns the values in the order of the names - return self._set_point.get_components(*names) + a, b, c = self._set_point.get_components(*names) + return a, b, c def _set_setpoint(self, name: str, value: float) -> None: self._set_setpoints((name,), (value,)) diff --git a/tests/drivers/test_ami430_visa.py b/tests/drivers/test_ami430_visa.py index 437da389c270..2203cbba5932 100644 --- a/tests/drivers/test_ami430_visa.py +++ b/tests/drivers/test_ami430_visa.py @@ -636,7 +636,7 @@ def test_measured( def test_component_and_vector_types(current_driver: AMIModel4303D) -> None: # single components are plain floats while the coordinate vectors are - # lists of three floats, both for the set points and the measured values + # tuples of three floats, both for the set points and the measured values current_driver.cartesian((0.1, 0.2, 0.3)) for name in _SINGLE_COMPONENTS: @@ -647,7 +647,7 @@ def test_component_and_vector_types(current_driver: AMIModel4303D) -> None: for name in _VECTORS: for parameter_name in (name, f"{name}_measured"): value = current_driver.parameters[parameter_name]() - assert isinstance(value, list), parameter_name + assert isinstance(value, tuple), parameter_name assert len(value) == 3 assert all(isinstance(component, float) for component in value) From d69d1d5fff4e1b89cb7198871bb4356fcbcbe2de Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 22:42:05 +0100 Subject: [PATCH 11/11] Return named tuples from AMI430 coordinate vectors Add CartesianVector, SphericalVector and CylindricalVector named tuples to qcodes.math_utils and return them from the AMI430 3D coordinate vector parameters, so the components can be accessed by name. They are tuples, so the previous tuple return still works. --- docs/changes/newsfragments/8679.breaking | 8 ++- docs/changes/newsfragments/8679.new | 3 + .../american_magnetics/AMI430_visa.py | 66 ++++++++++++------- src/qcodes/math_utils/__init__.py | 9 ++- src/qcodes/math_utils/field_vector.py | 35 +++++++++- tests/drivers/test_ami430_visa.py | 41 ++++++++++-- tests/test_field_vector.py | 23 +++++++ 7 files changed, 149 insertions(+), 36 deletions(-) create mode 100644 docs/changes/newsfragments/8679.new diff --git a/docs/changes/newsfragments/8679.breaking b/docs/changes/newsfragments/8679.breaking index 9ec4ba663b63..aa3b30145dfd 100644 --- a/docs/changes/newsfragments/8679.breaking +++ b/docs/changes/newsfragments/8679.breaking @@ -5,6 +5,8 @@ meaning by a factor of 10. Set the field units before creating the 3D driver, or driver first. The coordinate vectors of ``AMIModel4303D`` (``cartesian``, ``spherical``, ``cylindrical`` and -their ``*_measured`` versions) now return a tuple of three floats instead of a list. Code that -compares the result to a list, or modifies it, needs updating. Setting these parameters still -accepts lists, tuples and numpy arrays. +their ``*_measured`` versions) now return a named tuple (``CartesianVector``, +``SphericalVector`` or ``CylindricalVector`` from ``qcodes.math_utils``) instead of a list, so +the components can also be accessed by name, e.g. ``cartesian_measured().z``. Code that compares +the result to a list, or modifies it, needs updating. Setting these parameters still accepts +lists, tuples and numpy arrays. diff --git a/docs/changes/newsfragments/8679.new b/docs/changes/newsfragments/8679.new new file mode 100644 index 000000000000..507312a7c804 --- /dev/null +++ b/docs/changes/newsfragments/8679.new @@ -0,0 +1,3 @@ +Added the named tuples ``CartesianVector`` (``x``, ``y``, ``z``), ``SphericalVector`` +(``field``, ``theta``, ``phi``) and ``CylindricalVector`` (``rho``, ``phi``, ``z``) to +``qcodes.math_utils``, for the components of a field vector in a given coordinate system. diff --git a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py index 9747067cfa07..d89985792ef5 100644 --- a/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py +++ b/src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py @@ -18,7 +18,12 @@ VisaInstrument, VisaInstrumentKWArgs, ) -from qcodes.math_utils import FieldVector +from qcodes.math_utils import ( + CartesianVector, + CylindricalVector, + FieldVector, + SphericalVector, +) from qcodes.metadatable import normalize_snapshot_update from qcodes.utils.types import NumberType from qcodes.validators import Anything, Bool, Enum, Ints, Numbers @@ -36,6 +41,16 @@ CartesianFieldLimitFunction = Callable[[float, float, float], bool] +# the FieldVector components of each coordinate vector +_VECTOR_COMPONENTS: dict[ + type[CartesianVector | SphericalVector | CylindricalVector], + tuple[str, str, str], +] = { + CartesianVector: ("x", "y", "z"), + SphericalVector: ("r", "theta", "phi"), + CylindricalVector: ("rho", "phi", "z"), +} + class AMI430Exception(Exception): pass @@ -759,12 +774,10 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) # Get-only parameters that return a measured value - self.cartesian_measured: Parameter[tuple[float, float, float], Self] = ( - self.add_parameter( - "cartesian_measured", - get_cmd=partial(self._get_measured, ("x", "y", "z")), - unit="T", - ) + self.cartesian_measured: Parameter[CartesianVector, Self] = self.add_parameter( + "cartesian_measured", + get_cmd=partial(self._get_measured, CartesianVector), + unit="T", ) """Parameter cartesian_measured""" @@ -783,12 +796,10 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) """Parameter z_measured""" - self.spherical_measured: Parameter[tuple[float, float, float], Self] = ( - self.add_parameter( - "spherical_measured", - get_cmd=partial(self._get_measured, ("r", "theta", "phi")), - unit="T", - ) + self.spherical_measured: Parameter[SphericalVector, Self] = self.add_parameter( + "spherical_measured", + get_cmd=partial(self._get_measured, SphericalVector), + unit="T", ) """Parameter spherical_measured""" @@ -813,10 +824,10 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: ) """Parameter field_measured""" - self.cylindrical_measured: Parameter[tuple[float, float, float], Self] = ( + self.cylindrical_measured: Parameter[CylindricalVector, Self] = ( self.add_parameter( "cylindrical_measured", - get_cmd=partial(self._get_measured, ("rho", "phi", "z")), + get_cmd=partial(self._get_measured, CylindricalVector), unit="T", ) ) @@ -830,11 +841,12 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: """Parameter rho_measured""" # Get and set parameters for the set points of the coordinates. These - # return a tuple but can be set from any sequence or a numpy array. + # return a named tuple (CartesianVector, SphericalVector or + # CylindricalVector) but can be set from any sequence or a numpy array. self.cartesian: Parameter[Sequence[float] | npt.NDArray, Self] = ( self.add_parameter( "cartesian", - get_cmd=partial(self._get_setpoints, ("x", "y", "z")), + get_cmd=partial(self._get_setpoints, CartesianVector), set_cmd=partial(self._set_setpoints, ("x", "y", "z")), unit="T", vals=Anything(), @@ -872,7 +884,7 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: self.spherical: Parameter[Sequence[float] | npt.NDArray, Self] = ( self.add_parameter( "spherical", - get_cmd=partial(self._get_setpoints, ("r", "theta", "phi")), + get_cmd=partial(self._get_setpoints, SphericalVector), set_cmd=partial(self._set_setpoints, ("r", "theta", "phi")), vals=Anything(), docstring="Set point in spherical coordinates (field, theta, phi), " @@ -915,7 +927,7 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430: self.cylindrical: Parameter[Sequence[float] | npt.NDArray, Self] = ( self.add_parameter( "cylindrical", - get_cmd=partial(self._get_setpoints, ("rho", "phi", "z")), + get_cmd=partial(self._get_setpoints, CylindricalVector), set_cmd=partial(self._set_setpoints, ("rho", "phi", "z")), vals=Anything(), docstring="Set point in cylindrical coordinates (rho, phi, z), " @@ -1383,18 +1395,22 @@ def _get_measured_field_vector(self) -> FieldVector: def _get_measured_component(self, name: str) -> float: return self._get_measured_field_vector().get_components(name)[0] - def _get_measured(self, names: tuple[str, str, str]) -> tuple[float, float, float]: + def _get_measured[V: (CartesianVector, SphericalVector, CylindricalVector)]( + self, vector_type: type[V] + ) -> V: # get_components returns the values in the order of the names - a, b, c = self._get_measured_field_vector().get_components(*names) - return a, b, c + names = _VECTOR_COMPONENTS[vector_type] + return vector_type(*self._get_measured_field_vector().get_components(*names)) def _get_setpoint(self, name: str) -> float: return self._set_point.get_components(name)[0] - def _get_setpoints(self, names: tuple[str, str, str]) -> tuple[float, float, float]: + def _get_setpoints[V: (CartesianVector, SphericalVector, CylindricalVector)]( + self, vector_type: type[V] + ) -> V: # get_components returns the values in the order of the names - a, b, c = self._set_point.get_components(*names) - return a, b, c + names = _VECTOR_COMPONENTS[vector_type] + return vector_type(*self._set_point.get_components(*names)) def _set_setpoint(self, name: str, value: float) -> None: self._set_setpoints((name,), (value,)) diff --git a/src/qcodes/math_utils/__init__.py b/src/qcodes/math_utils/__init__.py index 90fc1f213e06..818471f18249 100644 --- a/src/qcodes/math_utils/__init__.py +++ b/src/qcodes/math_utils/__init__.py @@ -3,6 +3,11 @@ such as coordinate system mappings. """ -from .field_vector import FieldVector +from .field_vector import ( + CartesianVector, + CylindricalVector, + FieldVector, + SphericalVector, +) -__all__ = ["FieldVector"] +__all__ = ["CartesianVector", "CylindricalVector", "FieldVector", "SphericalVector"] diff --git a/src/qcodes/math_utils/field_vector.py b/src/qcodes/math_utils/field_vector.py index 837756523817..847631cf8aac 100644 --- a/src/qcodes/math_utils/field_vector.py +++ b/src/qcodes/math_utils/field_vector.py @@ -5,7 +5,7 @@ from __future__ import annotations -from typing import TYPE_CHECKING, Any, ClassVar, Literal +from typing import TYPE_CHECKING, Any, ClassVar, Literal, NamedTuple import numpy as np import numpy.typing as npt @@ -20,6 +20,39 @@ NormOrder = Literal["fro", "nuc"] | None | float +class CartesianVector(NamedTuple): + """ + The components of a field vector in Cartesian coordinates. + """ + + x: float + y: float + z: float + + +class SphericalVector(NamedTuple): + """ + The components of a field vector in spherical coordinates, with the angles + in degrees. ``field`` is the magnitude of the vector, called ``r`` in + :class:`FieldVector`. + """ + + field: float + theta: float + phi: float + + +class CylindricalVector(NamedTuple): + """ + The components of a field vector in cylindrical coordinates, with the angle + in degrees. + """ + + rho: float + phi: float + z: float + + class FieldVector: """ A convenient class to keep track of vectors representing physical fields. diff --git a/tests/drivers/test_ami430_visa.py b/tests/drivers/test_ami430_visa.py index 2203cbba5932..256ec5fef493 100644 --- a/tests/drivers/test_ami430_visa.py +++ b/tests/drivers/test_ami430_visa.py @@ -21,7 +21,12 @@ AMIModel430, AMIModel4303D, ) -from qcodes.math_utils import FieldVector +from qcodes.math_utils import ( + CartesianVector, + CylindricalVector, + FieldVector, + SphericalVector, +) from qcodes.utils.types import ( numpy_concrete_floats, numpy_concrete_ints, @@ -631,12 +636,17 @@ def test_measured( _SINGLE_COMPONENTS = ("x", "y", "z", "field", "theta", "phi", "rho") -_VECTORS = ("cartesian", "spherical", "cylindrical") +_VECTORS = { + "cartesian": CartesianVector, + "spherical": SphericalVector, + "cylindrical": CylindricalVector, +} def test_component_and_vector_types(current_driver: AMIModel4303D) -> None: # single components are plain floats while the coordinate vectors are - # tuples of three floats, both for the set points and the measured values + # named tuples of three floats, both for the set points and the measured + # values current_driver.cartesian((0.1, 0.2, 0.3)) for name in _SINGLE_COMPONENTS: @@ -644,14 +654,35 @@ def test_component_and_vector_types(current_driver: AMIModel4303D) -> None: value = current_driver.parameters[parameter_name]() assert isinstance(value, float), parameter_name - for name in _VECTORS: + for name, vector_type in _VECTORS.items(): for parameter_name in (name, f"{name}_measured"): value = current_driver.parameters[parameter_name]() - assert isinstance(value, tuple), parameter_name + assert type(value) is vector_type, parameter_name assert len(value) == 3 assert all(isinstance(component, float) for component in value) +def test_vector_components_by_name(current_driver: AMIModel4303D) -> None: + current_driver.cartesian((0.1, 0.2, 0.3)) + + cartesian = current_driver.cartesian_measured() + assert (cartesian.x, cartesian.y, cartesian.z) == pytest.approx((0.1, 0.2, 0.3)) + + spherical = current_driver.spherical() + # the settable vectors are typed as what they accept, so narrow the type + assert isinstance(spherical, SphericalVector) + assert spherical.field == pytest.approx(current_driver.field()) + assert spherical.theta == pytest.approx(current_driver.theta()) + assert spherical.phi == pytest.approx(current_driver.phi()) + + cylindrical = current_driver.cylindrical_measured() + assert cylindrical.rho == pytest.approx(current_driver.rho_measured()) + assert cylindrical.z == pytest.approx(0.3) + + # named tuples compare equal to plain tuples + assert current_driver.cartesian() == tuple(current_driver.cartesian()) + + def test_set_single_component(current_driver: AMIModel4303D) -> None: current_driver.cartesian((0.1, 0.2, 0.3)) diff --git a/tests/test_field_vector.py b/tests/test_field_vector.py index 16cb6f2b3765..2362078a2bcb 100644 --- a/tests/test_field_vector.py +++ b/tests/test_field_vector.py @@ -10,6 +10,11 @@ from hypothesis import given, settings from hypothesis.strategies import floats, tuples +from qcodes.math_utils import ( + CartesianVector, + CylindricalVector, + SphericalVector, +) from qcodes.math_utils.field_vector import FieldVector from qcodes.utils import NumpyJSONEncoder @@ -161,3 +166,21 @@ def test_all_attributes_are_floats() -> None: for attr in FieldVector.attributes: assert isinstance(getattr(fv, attr), float) + + +def test_coordinate_named_tuples() -> None: + vector = FieldVector(x=0.1, y=0.2, z=0.3) + + cartesian = CartesianVector(*vector.get_components("x", "y", "z")) + spherical = SphericalVector(*vector.get_components("r", "theta", "phi")) + cylindrical = CylindricalVector(*vector.get_components("rho", "phi", "z")) + + assert cartesian.x == 0.1 + assert spherical.field == vector.r + assert cylindrical.rho == vector.rho + # they are tuples, so they compare equal to and unpack like plain tuples + assert cartesian == (0.1, 0.2, 0.3) + x, y, z = cartesian + assert (x, y, z) == (0.1, 0.2, 0.3) + # and they are stored as plain lists in JSON, like other tuples + assert json.loads(json.dumps(cartesian, cls=NumpyJSONEncoder)) == [0.1, 0.2, 0.3]