Fix parameter units being silently changed to SI units by henrikjacobsenfys · Pull Request #311 · easyscience/core

henrikjacobsenfys · 2026-09-17T13:04:19Z

This was written partly by Claude under my guidance.

Handles the issue mentioned here: #300

The solution is twofold:

store the string users give when supplying a unit, and use that string rather than the scipp-generated one. This means users get the unit they asked for, not whichever unit combination the underlying library thinks it should be
strip numbers only when the unit is purely a number, e.g. in the m/mm example.

For 1, it also handles e.g. when parameters are added or subtracted from other parameters.

rozyczko · 2026-09-24T09:13:13Z

The design is good and the implementation is careful. There is one inconsistency worth fixing before merge, one API contract change that needs to be documented and communicated, and a substantial amount of duplicated code that should be factored out

The stored value depends on whether unit was passed as str or sc.Unit. Is this as expected?
The "should I normalise?" decision is currently inferred from the type of the constructor argument! This looks wrong.

DescriptorNumber('s', 1.0, 'dm*m').value              # 1.0    , unit 'dm*m'   (scipp 0.1m^2)
DescriptorNumber('s', 1.0, sc.Unit('dm*m')).value     # 0.1    , unit 'm^2'

DescriptorNumber('s', 1.0, '1/meV^2').value           # 1.0    , unit '1/meV^2'
DescriptorNumber('s', 1.0, sc.Unit('1/meV^2')).value  # 3.9e+43, unit '1/J^2'

.unit is no longer a key
.unit is now only a display string. Two objects with the same unit can report different strings:

DescriptorNumber('a', 1, 'm**-1').unit == DescriptorNumber('b', 1, '1/m').unit   # False
DescriptorNumber('a', 1, 'angstrom').unit == DescriptorNumber('b', 1, 'A').unit  # False
# but in both cases a._scalar.unit == b._scalar.unit  -> True

I guess this is "as designed", but the dependent modules, like reflectometry, might be comparing .unit strings to test unit compatibility. This will now fail for particular spellings...

The spelling methods are duplicated between the two descriptor classes

_remember_unit and _spelling_from_sources are identical apart from the type annotation style,
and also the two-line spelling check in each unit getter and the (state, _input_unit, _input_unit_parsed) tuple
handling. That is roughly 90 duplicated lines including docstrings.
Maybe create a mixin class?

dead imports
si_base_unit is imported at descriptor_number.py:26 and
descriptor_array.py:28 and used in neither. It is only used inside units.py itself.

rozyczko · 2026-09-24T09:17:50Z

-            self._convert_unit(self._base_unit())
+        # Make sure no magnitude is left hiding inside the unit. This has to happen after
+        # super().__init__, so that subclasses which convert more than the scalar (such
+        # as Parameter, with its bounds) are fully constructed. It should not be recorded


super().__init__ here is DescriptorBase.__init__, which knows nothing about Parameter!!

rozyczko · 2026-09-24T09:21:06Z

-    fitter : Fitter
-        A configured ``Fitter`` (or ``MultiFitter``) whose minimizer has been
-        switched to ``AvailableMinimizers.Bumps``.
+    fitter : 'Fitter'


The quotes around Fitter come from some linting tool and are not valid numpydoc syntax.

rozyczko · 2026-09-24T09:21:52Z

            try:
                for i in range(count):
                    param = Parameter(name=f'thread_{thread_id}_param_{i}', value=float(i))
+                    created.append(param)


errr why is this change here? Leftover from testing?

It's because Claude found that this test passed for the wrong reason

rozyczko · 2026-09-24T09:24:56Z

+        """
+        self._scalar, self._input_unit, self._input_unit_parsed = state
+
+    def _remember_unit(self, unit: str | sc.Unit | None) -> None:


descriptor_array.py version of this method uses typing Union

def _remember_unit(self, unit: Union[str, sc.Unit, None]) -> None:

standardize on one

I think we need to go through the entire library and update it - we still use Optional in some other places..

rozyczko · 2026-09-24T09:37:54Z

+        if sc.Unit(target) != base_unit:
+            # The target does not survive a round trip, so it is not safe to use.
+            return None
+    except Exception:


Bare exception is bad! Especially covering si_base_unit call, conversion to string and calling scipp's Unit. Please consider making the exception more specific.

henrikjacobsenfys · 2026-10-06T05:06:21Z

The design is good and the implementation is careful. There is one inconsistency worth fixing before merge, one API contract change that needs to be documented and communicated, and a substantial amount of duplicated code that should be factored out

The stored value depends on whether unit was passed as str or sc.Unit. Is this as expected?
The "should I normalise?" decision is currently inferred from the type of the constructor argument! This looks wrong.
DescriptorNumber('s', 1.0, 'dm*m').value              # 1.0    , unit 'dm*m'   (scipp 0.1m^2)
DescriptorNumber('s', 1.0, sc.Unit('dm*m')).value     # 0.1    , unit 'm^2'

DescriptorNumber('s', 1.0, '1/meV^2').value           # 1.0    , unit '1/meV^2'
DescriptorNumber('s', 1.0, sc.Unit('1/meV^2')).value  # 3.9e+43, unit '1/J^2'
.unit is no longer a key
.unit is now only a display string. Two objects with the same unit can report different strings:
DescriptorNumber('a', 1, 'm**-1').unit == DescriptorNumber('b', 1, '1/m').unit   # False
DescriptorNumber('a', 1, 'angstrom').unit == DescriptorNumber('b', 1, 'A').unit  # False
# but in both cases a._scalar.unit == b._scalar.unit  -> True
I guess this is "as designed", but the dependent modules, like reflectometry, might be comparing .unit strings to test unit compatibility. This will now fail for particular spellings...

The spelling methods are duplicated between the two descriptor classes

_remember_unit and _spelling_from_sources are identical apart from the type annotation style, and also the two-line spelling check in each unit getter and the (state, _input_unit, _input_unit_parsed) tuple handling. That is roughly 90 duplicated lines including docstrings. Maybe create a mixin class?

dead imports
si_base_unit is imported at descriptor_number.py:26 and
descriptor_array.py:28 and used in neither. It is only used inside units.py itself.

Yes, there's now explicitly a difference between using a string an a sc.Unit. A sc.Unit is stored in SI units underneath and has its own representation. There are some standard non-SI units included, see the discussion here: #300

A sc.Unit is the user saying "I want this unit, call it what you want".
A string is the user saying "THIS is how I want my unit represented. Not 1e-40 J^02 or whatever it corresponds to, I wrote "meV^-2" and that's what I want.

I'm not entirely sure what to do about "unit as a key" vs "unit as a display string". We can't really do both with just .unit. So we either choose that .unit stays a sc.Unit as it was and add some other way to see the unit in, well, the "correct" form, or it becomes a display string as it's suggested here, and we use .full_value.unit or something similar to get the underlying sc.Unit.

henrikjacobsenfys · 2026-10-06T06:15:35Z

I don't know why it keeps failing the lint format. pixi run fix is green locally

Fix parameter units being silently changed to SI units

d80952a

henrikjacobsenfys added [scope] bug Bug report or fix (major.minor.PATCH) [priority] medium Normal/default priority labels Sep 17, 2026

henrikjacobsenfys added 2 commits September 17, 2026 15:08

fix linting error

b8ad51e

fix errors

ca5b265

rozyczko requested changes Sep 24, 2026

View reviewed changes

henrikjacobsenfys added 2 commits October 6, 2026 07:00

Respond to reviewer comments

9a9ead7

pixi run fix

6b63afd

pixi run fix

c52523c

Navigation Menu

Repository navigation

Fix parameter units being silently changed to SI units - #311

Fix parameter units being silently changed to SI units#311
henrikjacobsenfys wants to merge 6 commits into
developfrom
fix-parameter-units

henrikjacobsenfys commented Sep 17, 2026 •

edited

Loading

Uh oh!

rozyczko commented Sep 24, 2026 •

edited

Loading

Uh oh!

rozyczko Sep 24, 2026

Uh oh!

rozyczko Sep 24, 2026

Uh oh!

rozyczko Sep 24, 2026

Uh oh!

henrikjacobsenfys Oct 6, 2026

Uh oh!

rozyczko Sep 24, 2026

Uh oh!

henrikjacobsenfys Oct 6, 2026

Uh oh!

rozyczko Sep 24, 2026

Uh oh!

henrikjacobsenfys commented Oct 6, 2026 •

edited

Loading

Uh oh!

henrikjacobsenfys commented Oct 6, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

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

Uh oh!

Uh oh!

rozyczko commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rozyczko Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

rozyczko Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

rozyczko Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

henrikjacobsenfys Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

rozyczko Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

henrikjacobsenfys Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

rozyczko Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

henrikjacobsenfys commented Oct 6, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

henrikjacobsenfys commented Sep 17, 2026 •

edited

Loading

rozyczko commented Sep 24, 2026 •

edited

Loading

henrikjacobsenfys commented Oct 6, 2026 •

edited

Loading