Added desired units and tests by henrikjacobsenfys · Pull Request #188 · easyscience/corelib

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.24%. Comparing base (bd10653) to head (5826b23).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #188      +/-   ##
===========================================
+ Coverage    81.15%   81.24%   +0.08%     
===========================================
  Files           52       52              
  Lines         4267     4287      +20     
  Branches       740      745       +5     
===========================================
+ Hits          3463     3483      +20     
  Misses         624      624              
  Partials       180      180

Flag	Coverage Δ
unittests	`81.24% <100.00%> (+0.08%)`	⬆️

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.79% <100.00%> (+0.30%)`	⬆️

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.

I didn't realise dependency_result changed self. Adding _revert_dependency() before raising the error.

src/easyscience/variable/parameter.py

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.

Added validation.

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).

Good point! Expanded the test_dependent_parameter_make_dependent_on_with_desired_unit test

henrikjacobsenfys · 2026-01-21T10:40:47Z

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.

Because all other Parameter constructors accept unit and make it so that the Parameter has the chosen unit I'm in favor of keeping the name here as well. It makes sense to me that the unit of the Parameter is unit, also if it's dependent.

If you/the the team prefer a different name (desired_unit or preferred_unit ?) should we then still allow unit as a kwarg? I feel like that would potentially be confusing to users, since they'd get no error or warning with this code:

b = Parameter.from_dependency(
             name="b",
             dependency_expression="2 * a",
             dependency_map={"a": a},
             unit="m",
             desired_unit = "cm"
         )

I'm also curious to hear what @rozyczko and @AndrewSazonov think about this :)

damskii9992 · 2026-02-02T13:02:47Z

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.
Because all other Parameter constructors accept unit and make it so that the Parameter has the chosen unit I'm in favor of keeping the name here as well. It makes sense to me that the unit of the Parameter is unit, also if it's dependent.

If you/the the team prefer a different name (desired_unit or preferred_unit ?) should we then still allow unit as a kwarg? I feel like that would potentially be confusing to users, since they'd get no error or warning with this code:
b = Parameter.from_dependency(
             name="b",
             dependency_expression="2 * a",
             dependency_map={"a": a},
             unit="m",
             desired_unit = "cm"
         )
I'm also curious to hear what @rozyczko and @AndrewSazonov think about this :)

The problem is that the unit that the user passes here will not be any unit like it can be for any other Parameter constructor.
In the regular Parameter constructor, the unit is the actual unit of the parameter, it gets set to this value.
When making a dependent parameter, the user cannot set the value of the unit, as it is inferred from the dependency expression (like the value, min/max and error). So if the user want e.g. "Meter" and the dependency expression returns a "Second" unit, the unit cannot be converted and it would result in an error, completely different from the regular unit setters.

We pass kwargs to the Parameter constructor for stuff like display_name, url, unique_name etc. For all the values inferred by the dependency evaluation, whatever the user provides gets overwritten. The user can also provide a "value", but just like the "unit", this kwarg simply gets passed on and overwritten immediately. This is just a side-effect of allowing arbitrary kwargs to be passed to the underlying Parameter constructor.

And this is why I want this "desired_unit" or "preffered_unit" to be named differently, because it is different from the regular "unit".

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

henrikjacobsenfys added 2 commits January 21, 2026 11:57

Update based on PR comments

05be062

Added a small test

5826b23

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

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!

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!

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

Uh oh!

Uh oh!

damskii9992 commented Feb 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jan 9, 2026 •

edited

Loading

henrikjacobsenfys commented Jan 20, 2026 •

edited

Loading

henrikjacobsenfys commented Jan 21, 2026 •

edited

Loading