From 6cf824cc931558249ed71975e09021d4e0597d11 Mon Sep 17 00:00:00 2001 From: Paul Baksic Date: Mon, 20 Jul 2026 14:23:25 +0200 Subject: [PATCH] Change list into tuple to avoid any side effect of not copied lists --- bindings/Sofa/package/Units/Core.py | 32 ++++--- bindings/Sofa/package/Units/tests/conftest.py | 22 +++-- .../Units/tests/test_simulation_parameters.py | 92 +++++++++---------- .../Sofa/package/Units/tests/test_units.py | 55 ++++++----- 4 files changed, 106 insertions(+), 95 deletions(-) diff --git a/bindings/Sofa/package/Units/Core.py b/bindings/Sofa/package/Units/Core.py index 468aaea3..340cfcef 100644 --- a/bindings/Sofa/package/Units/Core.py +++ b/bindings/Sofa/package/Units/Core.py @@ -1,8 +1,8 @@ import math class Unit(): - numerator : list - denumerator : list + numerator : tuple + denumerator : tuple ratio : float @@ -98,8 +98,8 @@ def __hash__(self): class NeutralUnit(Unit): def __init__(self): - self.numerator = [] - self.denumerator = [] + self.numerator = () + self.denumerator = () self.ratio = 1.0 def __str__(self): @@ -112,17 +112,17 @@ class PrimaryUnit(Unit): def __init__(self, abrev : str): self.abrev = abrev - self.numerator = [self] - self.denumerator = [] + self.numerator = (self,) + self.denumerator = () self.ratio = 1.0 class DerivedUnit(Unit): - def __init__(self, numerator : list[PrimaryUnit], denumerator : list[PrimaryUnit], ratio : float): - self.numerator = numerator - self.denumerator = denumerator + def __init__(self, numerator : tuple[PrimaryUnit], denumerator : tuple[PrimaryUnit], ratio : float): + self.numerator = tuple(numerator) + self.denumerator = tuple(denumerator) self.ratio = ratio self.simplify() @@ -130,23 +130,25 @@ def __init__(self, numerator : list[PrimaryUnit], denumerator : list[PrimaryUnit def simplify(self): futNum = [] + denum = list(self.denumerator) for unit in self.numerator: simplified = False - for i in range(len(self.denumerator)): - if self.denumerator[i].abrev == unit.abrev: + for i in range(len(denum)): + if denum[i].abrev == unit.abrev: simplified = True - self.denumerator.pop(i) + denum.pop(i) break if not(simplified): futNum.append(unit) - self.numerator = futNum + self.numerator = tuple(futNum) + self.denumerator = tuple(denum) class ScaledUnit(Unit): def __init__(self, unit : Unit, ratio : float): - self.numerator = unit.numerator.copy() - self.denumerator = unit.denumerator.copy() + self.numerator = unit.numerator + self.denumerator = unit.denumerator self.ratio = ratio diff --git a/bindings/Sofa/package/Units/tests/conftest.py b/bindings/Sofa/package/Units/tests/conftest.py index db30d8c5..cd2ed35b 100644 --- a/bindings/Sofa/package/Units/tests/conftest.py +++ b/bindings/Sofa/package/Units/tests/conftest.py @@ -1,17 +1,21 @@ """ -Allows the test files in this folder to `import units` and -`import SimulationParameters` even though this folder is a sub-folder of -the project root (where those two modules actually live). +Makes the `Units` package importable by the test files in this folder. + +The modules under test (`Core`, `Definitions`, `UnitSystem`) use +package-relative imports (`from .Core import *`), so they must be imported +through the `Units` package rather than as top-level modules. This folder is +`Units/tests`, so the package's parent directory is two levels up; inserting +it into sys.path lets the tests do `from Units.Core import ...`. pytest imports conftest.py before collecting/importing the test modules in -the same directory, so inserting the parent folder into sys.path here is -enough - the test files themselves don't need to know anything about the -project layout. +the same directory, so doing this here is enough. """ import os import sys -_PROJECT_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), os.pardir)) +_PACKAGE_PARENT = os.path.abspath( + os.path.join(os.path.dirname(__file__), os.pardir, os.pardir) +) -if _PROJECT_ROOT not in sys.path: - sys.path.insert(0, _PROJECT_ROOT) +if _PACKAGE_PARENT not in sys.path: + sys.path.insert(0, _PACKAGE_PARENT) diff --git a/bindings/Sofa/package/Units/tests/test_simulation_parameters.py b/bindings/Sofa/package/Units/tests/test_simulation_parameters.py index 33f4163b..e2514915 100644 --- a/bindings/Sofa/package/Units/tests/test_simulation_parameters.py +++ b/bindings/Sofa/package/Units/tests/test_simulation_parameters.py @@ -1,110 +1,110 @@ """ -Unit tests for SimulationParameters.py +Unit tests for UnitSystem.py Run with: pytest test_simulation_parameters.py -v """ import pytest -from Definitions import s, m, mm, ms, kg, N, Pa, kPa, kN, tho -from SimulationParameters import BaseParameterSet, SOFAParameters +from Units.Definitions import s, m, mm, ms, kg, N, Pa, kPa, kN, tau +from Units.UnitSystem import UnitSystem, MechanicalUnitSystem # --------------------------------------------------------------------------- -# BaseParameterSet construction +# UnitSystem construction # --------------------------------------------------------------------------- -class TestBaseParameterSetConstruction: +class TestUnitSystemConstruction: def test_positional_primary_units_are_registered(self): - bp = BaseParameterSet(s, m, kg) + bp = UnitSystem(s, m, kg) assert set(bp.units.keys()) == {"s", "m", "kg"} assert bp.units["s"] is s assert bp.units["m"] is m assert bp.units["kg"] is kg def test_keyword_primary_units_are_registered(self): - bp = BaseParameterSet(time=s, length=mm, mass=kg) + bp = UnitSystem(time=s, length=mm, mass=kg) assert set(bp.units.keys()) == {"s", "m", "kg"} assert bp.units["m"] is mm def test_non_unit_positional_args_are_ignored(self): - bp = BaseParameterSet(s, "not a unit", 42) + bp = UnitSystem(s, "not a unit", 42) assert set(bp.units.keys()) == {"s"} def test_non_unit_keyword_args_are_ignored(self): - bp = BaseParameterSet(time=s, other="not a unit") + bp = UnitSystem(time=s, other="not a unit") assert set(bp.units.keys()) == {"s"} def test_derived_unit_positional_arg_raises_type_error(self): with pytest.raises(TypeError): - BaseParameterSet(N) + UnitSystem(N) def test_derived_unit_keyword_arg_raises_type_error(self): with pytest.raises(TypeError): - BaseParameterSet(force=N) + UnitSystem(force=N) def test_empty_construction_gives_empty_units(self): - bp = BaseParameterSet() + bp = UnitSystem() assert bp.units == {} # --------------------------------------------------------------------------- -# BaseParameterSet.convert() +# UnitSystem.convert() # --------------------------------------------------------------------------- class TestConvert: def test_convert_with_all_base_units_matching(self): - bp = BaseParameterSet(s, m, kg) + bp = UnitSystem(s, m, kg) assert bp.convert(10, N) == pytest.approx(10.0) def test_convert_scales_with_position_unit(self): # Simulation uses millimeters for position: 1 "simulation force unit" # corresponds to 1000 N because of the mm scaling. - bp = BaseParameterSet(s, mm, kg) + bp = UnitSystem(s, mm, kg) assert bp.convert(10, N) == pytest.approx(10000.0) def test_convert_pressure_with_base_units(self): - bp = BaseParameterSet(s, m, kg) + bp = UnitSystem(s, m, kg) assert bp.convert(10, kPa) == pytest.approx(10000.0) def test_convert_pressure_with_mm_position(self): - bp = BaseParameterSet(s, mm, kg) + bp = UnitSystem(s, mm, kg) assert bp.convert(10, kPa) == pytest.approx(10.0) def test_convert_raises_when_required_unit_missing(self): - bp = BaseParameterSet(s, kg) # no 'm' defined + bp = UnitSystem(s, kg) # no 'm' defined with pytest.raises(RuntimeError): bp.convert(1, N) def test_convert_raises_mentions_missing_unit_abrev(self): - bp = BaseParameterSet(s, kg) + bp = UnitSystem(s, kg) with pytest.raises(RuntimeError, match="m"): bp.convert(1, N) # --------------------------------------------------------------------------- -# SOFAParameters +# MechanicalUnitSystem # --------------------------------------------------------------------------- -class TestSOFAParameters: +class TestMechanicalUnitSystem: def test_defaults_are_s_m_kg(self): - sp = SOFAParameters() + sp = MechanicalUnitSystem() assert sp.units['s'] is s assert sp.units['m'] is m assert sp.units['kg'] is kg assert set(sp.units.keys()) == {"s", "m", "kg"} def test_custom_units_are_stored_as_attributes(self): - sp = SOFAParameters(time=s, length=mm, mass=kg) + sp = MechanicalUnitSystem(time=s, length=mm, mass=kg) assert sp.units['m'] is mm assert set(sp.units.keys()) == {"s", "m", "kg"} def test_convert_uses_configured_units(self): - sp = SOFAParameters(time=s, length=mm, mass=kg) + sp = MechanicalUnitSystem(time=s, length=mm, mass=kg) assert sp.convert(10, N) == pytest.approx(10000.0) assert sp.convert(10, kPa) == pytest.approx(10.0) def test_convert_with_default_units(self): - sp = SOFAParameters() + sp = MechanicalUnitSystem() assert sp.convert(10, N) == pytest.approx(10.0) assert sp.convert(10, kPa) == pytest.approx(10000.0) @@ -114,24 +114,24 @@ class TestConvertExponents: def test_convert_force_with_scaled_time_unit(self): - bp = BaseParameterSet(ms, m, kg) + bp = UnitSystem(ms, m, kg) # 1 sim force unit = 1 kg * 1 m / (1 ms)^2 = 1e6 N # -> converting 1 N (SI) into sim units should give 1e-6 assert bp.convert(1, N) == pytest.approx(1e-6) def test_convert_torque_with_scaled_length_unit(self): - bp = BaseParameterSet(s, mm, kg) - # 1 sim torque unit = 1 kg * (1 mm)^2 / (1 s)^2 = 1e-6 tho - # -> converting 1 tho (SI) into sim units should give 1e6 - assert bp.convert(1, tho) == pytest.approx(1e6) + bp = UnitSystem(s, mm, kg) + # 1 sim torque unit = 1 kg * (1 mm)^2 / (1 s)^2 = 1e-6 tau + # -> converting 1 tau (SI) into sim units should give 1e6 + assert bp.convert(1, tau) == pytest.approx(1e6) def test_convert_when_scaled_dimension_only_has_exponent_one(self): # Sanity check / contrast case: this is why the bug doesn't show # up anywhere in SimulationParameters.py as shipped - position is # scaled (mm) but only ever appears as m^1 in N and Pa, so the # missing exponent handling never bites here. - bp = BaseParameterSet(s, mm, kg) + bp = UnitSystem(s, mm, kg) assert bp.convert(10, N) == pytest.approx(10000.0) assert bp.convert(10, kPa) == pytest.approx(10.0) @@ -141,53 +141,53 @@ def test_convert_when_scaled_dimension_only_has_exponent_one(self): # =========================================================================== import numpy as np -from Definitions import g, v, DimensionLess +from Units.Definitions import g, v, DimensionLess class TestDuplicateAndMixedRegistration: def test_duplicate_primary_unit_raises_value_error(self): with pytest.raises(ValueError): - BaseParameterSet(s, s) + UnitSystem(s, s) def test_scaled_and_plain_of_same_dimension_raises(self): # mm and m share the abbreviation "m" -> second one must be refused with pytest.raises(ValueError): - BaseParameterSet(m, mm) + UnitSystem(m, mm) def test_scaled_unit_is_accepted_as_primary(self): - bp = BaseParameterSet(mm) + bp = UnitSystem(mm) assert bp.units["m"] is mm class TestConvertMoreCases: def test_convert_scaled_target_unit_uses_its_ratio(self): - bp = BaseParameterSet(s, m, kg) + bp = UnitSystem(s, m, kg) assert bp.convert(1, kN) == pytest.approx(1000.0) def test_convert_scaled_target_against_matching_scaled_base(self): - bp = BaseParameterSet(s, mm, kg) + bp = UnitSystem(s, mm, kg) assert bp.convert(1, mm) == pytest.approx(1.0) def test_convert_with_scaled_mass_unit(self): # base mass = gram: 1 sim force unit = 1 g*m/s^2 = 1e-3 N - bp = BaseParameterSet(s, m, g) + bp = UnitSystem(s, m, g) assert bp.convert(1, N) == pytest.approx(1e3) def test_convert_dimensionless_is_identity(self): - bp = BaseParameterSet(s, m, kg) + bp = UnitSystem(s, m, kg) assert bp.convert(5, DimensionLess) == pytest.approx(5.0) def test_convert_missing_denominator_unit_raises(self): - bp = BaseParameterSet(m, kg) # no 's' defined, v = m/s needs it + bp = UnitSystem(m, kg) # no 's' defined, v = m/s needs it with pytest.raises(RuntimeError, match="s"): bp.convert(1, v) def test_convert_zero_value(self): - bp = BaseParameterSet(s, mm, kg) + bp = UnitSystem(s, mm, kg) assert bp.convert(0, N) == pytest.approx(0.0) def test_convert_negative_value(self): - bp = BaseParameterSet(s, mm, kg) + bp = UnitSystem(s, mm, kg) assert bp.convert(-1, N) == pytest.approx(-1000.0) @@ -196,7 +196,7 @@ class TestCallDispatch: lists and numpy arrays.""" def setup_method(self): - self.bp = BaseParameterSet(s, mm, kg) # 1 N -> 1000 sim units + self.bp = UnitSystem(s, mm, kg) # 1 N -> 1000 sim units def test_call_with_dimensionned_value(self): assert self.bp(10 * N) == pytest.approx(10000.0) @@ -238,16 +238,16 @@ def test_call_with_three_args_raises_value_error(self): self.bp(1.0, N, N) -class TestSOFAParametersMore: +class TestMechanicalUnitSystemMore: def test_positional_override(self): - sp = SOFAParameters(ms, mm) + sp = MechanicalUnitSystem(ms, mm) assert sp.units["s"] is ms assert sp.units["m"] is mm assert sp.units["kg"] is kg def test_mixed_scaled_units_conversion(self): # time in ms, length in mm: 1 sim force = kg*mm/ms^2 = 1000 N - sp = SOFAParameters(time=ms, length=mm) + sp = MechanicalUnitSystem(time=ms, length=mm) assert sp.convert(1, N) == pytest.approx(1e-3) diff --git a/bindings/Sofa/package/Units/tests/test_units.py b/bindings/Sofa/package/Units/tests/test_units.py index 28c3a153..084b4782 100644 --- a/bindings/Sofa/package/Units/tests/test_units.py +++ b/bindings/Sofa/package/Units/tests/test_units.py @@ -10,9 +10,9 @@ import math import pytest -from Definitions import ( +from Units.Definitions import ( DimensionLess, m, s, kg, - v, a, N, Pa, tho, + v, a, N, Pa, tau, nm, mm, cm, km, ms, µs, g, mg, t, @@ -20,7 +20,7 @@ kPa, MPa, ) -from Core import ( +from Units.Core import ( Unit, NeutralUnit, PrimaryUnit, DerivedUnit, ScaledUnit, DimensionnedValue ) @@ -36,8 +36,8 @@ def test_abrev_is_stored(self): assert kg.abrev == "kg" def test_primary_unit_is_its_own_numerator(self): - assert m.numerator == [m] - assert m.denumerator == [] + assert m.numerator == (m,) + assert m.denumerator == () def test_primary_unit_ratio_is_one(self): assert m.ratio == 1.0 @@ -45,15 +45,15 @@ def test_primary_unit_ratio_is_one(self): def test_new_primary_unit_construction(self): x = PrimaryUnit("x") assert x.abrev == "x" - assert x.numerator == [x] - assert x.denumerator == [] + assert x.numerator == (x,) + assert x.denumerator == () assert x.ratio == 1.0 class TestNeutralUnit: def test_neutral_unit_has_no_num_or_denum(self): - assert DimensionLess.numerator == [] - assert DimensionLess.denumerator == [] + assert DimensionLess.numerator == () + assert DimensionLess.denumerator == () def test_neutral_unit_ratio_is_one(self): assert DimensionLess.ratio == 1.0 @@ -62,7 +62,7 @@ def test_multiplying_by_neutral_unit_is_identity(self): result = DimensionLess * m assert result.ratio == 1.0 assert [u.abrev for u in result.numerator] == ["m"] - assert result.denumerator == [] + assert result.denumerator == () # --------------------------------------------------------------------------- @@ -102,7 +102,7 @@ def test_multiply_two_primary_units(self): result = m * m assert isinstance(result, DerivedUnit) assert [u.abrev for u in result.numerator] == ["m", "m"] - assert result.denumerator == [] + assert result.denumerator == () assert result.ratio == 1.0 def test_multiply_combines_ratios(self): @@ -139,15 +139,15 @@ class TestSimplify: def test_simplify_cancels_matching_units(self): # (m/s) * (s/m) should fully cancel to a dimensionless unit result = (m / s) * (s / m) - assert result.numerator == [] - assert result.denumerator == [] + assert result.numerator == () + assert result.denumerator == () assert result.ratio == 1.0 def test_simplify_cancels_only_one_occurrence(self): # (m*m) / m -> should cancel exactly one 'm', leaving one 'm' in numerator result = (m * m) / m assert [u.abrev for u in result.numerator] == ["m"] - assert result.denumerator == [] + assert result.denumerator == () def test_simplify_does_not_cancel_unmatched_units(self): # N has kg (no matching denum) and m (no matching denum); s^2 in @@ -169,14 +169,19 @@ def test_ratio_is_the_scale_factor(self): def test_scaled_unit_keeps_base_unit_dimension(self): assert [u.abrev for u in mm.numerator] == ["m"] - assert mm.denumerator == [] + assert mm.denumerator == () assert [u.abrev for u in kN.numerator] == ["kg", "m"] assert [u.abrev for u in kN.denumerator] == ["s", "s"] - def test_scaled_unit_doesnt_shares_list_reference_with_base_unit(self): - assert mm.numerator is not m.numerator - assert kN.numerator is not N.numerator - assert kN.denumerator is not N.denumerator + def test_scaled_unit_shares_immutable_dimension_with_base_unit(self): + # numerator/denumerator are tuples (immutable), so ScaledUnit can + # safely share the base unit's objects - there is no longer any risk + # of one mutating the other, so a defensive copy is unnecessary. + assert isinstance(mm.numerator, tuple) + assert isinstance(kN.denumerator, tuple) + assert mm.numerator == m.numerator + assert kN.numerator == N.numerator + assert kN.denumerator == N.denumerator # --------------------------------------------------------------------------- @@ -356,14 +361,14 @@ def test_scaled_unit_of_derived_unit_keeps_dimension(self): class TestSimplifyMutation: - def test_constructor_mutates_callers_denominator_list(self): - # BUG/quirk: DerivedUnit.simplify() pops from the very list object - # the caller passed in, so the caller's list is emptied as a side - # effect. The operators (*, /, **) always build fresh lists so they - # are unaffected, but direct construction is not safe. + def test_constructor_does_not_mutate_callers_lists(self): + # FIXED: DerivedUnit copies its arguments into tuples, so simplify() + # works on internal copies only. Constructing a unit no longer empties + # the caller's lists as a side effect. num, den = [m], [m] DerivedUnit(numerator=num, denumerator=den, ratio=1.0) - assert den == [] # caller's list was mutated in place + assert num == [m] + assert den == [m] # caller's list is left intact # ---------------------------------------------------------------------------