Added desired units and tests #188

henrikjacobsenfys · 2026-01-09T14:01:40Z

Added desired units for the dependent parameter constructors, and a method to change it later: set_desired_unit

codecov · 2026-01-09T14:01:46Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.22%. Comparing base (bd10653) to head (5d5199f).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #188      +/-   ##
===========================================
+ Coverage    81.15%   81.22%   +0.07%     
===========================================
  Files           52       52              
  Lines         4267     4283      +16     
  Branches       740      744       +4     
===========================================
+ Hits          3463     3479      +16     
  Misses         624      624              
  Partials       180      180

Flag	Coverage Δ
unittests	`81.22% <100.00%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/easyscience/variable/parameter.py	`90.73% <100.00%> (+0.24%)`	⬆️

github-actions

This pull request does not contain a valid label. Please add one of the following labels: ['[scope] bug', '[scope] enhancement', '[scope] documentation', '[scope] significant', '[scope] maintenance']

rozyczko · 2026-01-09T14:58:05Z

src/easyscience/variable/parameter.py

+                try:
+                    self._convert_unit(self._desired_unit)
+                except Exception as e:
+                    raise UnitError(f'Failed to convert unit from {temporary_parameter.unit} to {self._desired_unit}: {e}')


Should we completely bail out on bad unit conversion? Maybe return the original value with the original unit and warn users of the failure of the unit conversion.

Hmm, good question. I tend to think yes, since something must have gone wrong if the user expects a unit that is incompatible with the calculated unit.

I would move this check down to the make_dependent_on method and do a _revert_dependency if it fails.
Like with all our other checks. The update method should have no checks.
The dependency_result can be converted to try it out.

I moved the check. Update only checks if self._desired_unit is not None

damskii9992

I wouldn't simply call this argument "unit", it is too vague and can be understood as it can be anything. Maybe "unit_format"? Or something similar.

damskii9992 · 2026-01-19T12:21:46Z

src/easyscience/variable/parameter.py

+
+        if self._independent:
+            raise AttributeError('This is an independent parameter, desired unit can only be set for dependent parameters.')
+        self._desired_unit = unit_str


This is missing a check on the type of unit_str

damskii9992 · 2026-01-19T12:44:24Z

src/easyscience/variable/parameter.py

+                try:
+                    self._convert_unit(self._desired_unit)
+                except Exception as e:
+                    raise UnitError(f'Failed to convert unit from {temporary_parameter.unit} to {self._desired_unit}: {e}')


I would move this check down to the make_dependent_on method and do a _revert_dependency if it fails.
Like with all our other checks. The update method should have no checks.
The dependency_result can be converted to try it out.

henrikjacobsenfys · 2026-01-20T13:04:46Z

I wouldn't simply call this argument "unit", it is too vague and can be understood as it can be anything. Maybe "unit_format"? Or something similar.

The argument 'unit' is already used at least in tests, e.g. the following, so I'm hesitant to change it. Thoughts?

    def test_dependent_parameter_serialization(self, clear_global_map):
        """Test serialization of parameters with dependencies."""
        # Create independent parameter
        a = Parameter(name="a", value=2.0, unit="m", min=0, max=10)
        
        # Create dependent parameter
        b = Parameter.from_dependency(
            name="b",
            dependency_expression="2 * a",
            dependency_map={"a": a},
            unit="m"
        )

damskii9992 · 2026-01-20T13:12:22Z

I wouldn't simply call this argument "unit", it is too vague and can be understood as it can be anything. Maybe "unit_format"? Or something similar.

The argument 'unit' is already used at least in tests, e.g. the following, so I'm hesitant to change it. Thoughts?
    def test_dependent_parameter_serialization(self, clear_global_map):
        """Test serialization of parameters with dependencies."""
        # Create independent parameter
        a = Parameter(name="a", value=2.0, unit="m", min=0, max=10)
        
        # Create dependent parameter
        b = Parameter.from_dependency(
            name="b",
            dependency_expression="2 * a",
            dependency_map={"a": a},
            unit="m"
        )

Why does it matter that it is already used in tests? O.o That would literally be the case for any name changes we do XD

henrikjacobsenfys · 2026-01-20T13:16:51Z

I wouldn't simply call this argument "unit", it is too vague and can be understood as it can be anything. Maybe "unit_format"? Or something similar.

The argument 'unit' is already used at least in tests, e.g. the following, so I'm hesitant to change it. Thoughts?
    def test_dependent_parameter_serialization(self, clear_global_map):
        """Test serialization of parameters with dependencies."""
        # Create independent parameter
        a = Parameter(name="a", value=2.0, unit="m", min=0, max=10)
        
        # Create dependent parameter
        b = Parameter.from_dependency(
            name="b",
            dependency_expression="2 * a",
            dependency_map={"a": a},
            unit="m"
        )
Why does it matter that it is already used in tests? O.o That would literally be the case for any name changes we do XD

Well it might not matter, it's just that that implies that people might rely on this behavior. But maybe we shouldn't allow a unit in from_dependency then? Since the only reason people would give a unit is because they expect the Parameter to have that unit, no?

Edit: the point is that from_dependency has always accepted unit as an argument, until this PR it just didn't do anything. Now it (tries) to set the unit of the dependent Parameter to be unit.

damskii9992 · 2026-01-20T13:26:25Z

I wouldn't simply call this argument "unit", it is too vague and can be understood as it can be anything. Maybe "unit_format"? Or something similar.

The argument 'unit' is already used at least in tests, e.g. the following, so I'm hesitant to change it. Thoughts?
    def test_dependent_parameter_serialization(self, clear_global_map):
        """Test serialization of parameters with dependencies."""
        # Create independent parameter
        a = Parameter(name="a", value=2.0, unit="m", min=0, max=10)
        
        # Create dependent parameter
        b = Parameter.from_dependency(
            name="b",
            dependency_expression="2 * a",
            dependency_map={"a": a},
            unit="m"
        )
Why does it matter that it is already used in tests? O.o That would literally be the case for any name changes we do XD
Well it might not matter, it's just that that implies that people might rely on this behavior. But maybe we shouldn't allow a unit in from_dependency then? Since the only reason people would give a unit is because they expect the Parameter to have that unit, no?

Edit: the point is that from_dependency has always accepted unit as an argument, until this PR it just didn't do anything. Now it (tries) to set the unit of the dependent Parameter to be unit.

Ahh, in this case it is simply the test that is wrong. The "unit" is a kwarg passed to the Parameter constructor, but it gets overwritten so it is a redundant argument.

rozyczko

A few issues noted.

rozyczko · 2026-01-20T13:54:43Z

src/easyscience/variable/parameter.py

+
+        if self._desired_unit is not None:
+            try:
+                dependency_result._convert_unit(self._desired_unit)
+            except Exception as e:
+                raise UnitError(f'Failed to convert unit from {dependency_result.unit} to {self._desired_unit}: {e}')
+


When the unit conversion fails, the code raises a UnitError but does not call _revert_dependency() first, leaving the parameter in an inconsistent state if unit conversion fails.
This is inconsistent with all other validation checks in make_dependent_on() that properly revert the dependency state before raising.

rozyczko · 2026-01-20T14:04:38Z

src/easyscience/variable/parameter.py

+    def set_desired_unit(self, unit_str: str) -> None:
+        """
+        Set the desired unit for a dependent Parameter. This will convert the parameter to the desired unit.


make_dependent_on and from_dependency methods accept unit: str | sc.Unit | None but this only takes str - is this expected?

rozyczko · 2026-01-20T14:06:29Z

src/easyscience/variable/parameter.py

+        self, dependency_expression: str, dependency_map: Optional[dict] = None, unit: str | sc.Unit | None = None
+    ) -> None:
        """
        Make this parameter dependent on another parameter. This will overwrite the current value, unit, variance, min and max.


There's no validation on unit type. A user can pass unit=1234 and this will get accepted.

rozyczko · 2026-01-20T14:08:18Z

src/easyscience/variable/parameter.py

                '_dependency_string': self._dependency_string,
                '_dependency_map': self._dependency_map,
                '_dependency_interpreter': self._dependency_interpreter,
                '_clean_dependency_string': self._clean_dependency_string,


You forgot to add _desired_unit to previous dependency

rozyczko · 2026-01-20T14:12:26Z

src/easyscience/variable/parameter.py

+                self._convert_unit(self._desired_unit)
+


_convert_unit doesn't catch any exceptions and we aren't checking anything here. Maybe add some exception handling so it doesn't bubble up potentially leaving the parameter in a weird state?

This is on purpose. To keep the _update method as fast as possible, all the checks are made in the make_dependent_on method. If this method succeeds, then the _update will also always succeed.
This is also how we did it for the other updates :)

rozyczko · 2026-01-20T14:13:40Z

tests/unit_tests/variable/test_parameter.py

        normal_parameter.value == 4
        self.compare_parameters(normal_parameter, 2*independent_parameter)

+


There's no test verifying that _desired_unit is properly handled when calling make_dependent_on on an already-dependent parameter (i.e., overwriting an existing dependency).

henrikjacobsenfys added 2 commits January 9, 2026 14:58

Added desired units and tests

de923ea

small update

585305e

henrikjacobsenfys requested review from damskii9992 and rozyczko January 9, 2026 14:01

github-actions bot requested changes Jan 9, 2026

View reviewed changes

rozyczko reviewed Jan 9, 2026

View reviewed changes

damskii9992 requested changes Jan 19, 2026

View reviewed changes

Update based on PR comments

9202613

Added a few tests

5d5199f

rozyczko requested changes Jan 20, 2026

View reviewed changes

		normal_parameter.value == 4
		self.compare_parameters(normal_parameter, 2*independent_parameter)

Added desired units and tests #188

Are you sure you want to change the base?

Added desired units and tests #188

Conversation

henrikjacobsenfys commented Jan 9, 2026

Uh oh!

codecov bot commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damskii9992 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrikjacobsenfys commented Jan 20, 2026

Uh oh!

damskii9992 commented Jan 20, 2026

Uh oh!

henrikjacobsenfys commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damskii9992 commented Jan 20, 2026

Uh oh!

rozyczko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jan 9, 2026 •

edited

Loading

henrikjacobsenfys commented Jan 20, 2026 •

edited

Loading