Raise `UnitError` when passing `unit=None` in `.to()` for objects with unit, avoiding silent no-op by jokasimr · Pull Request #3945 · scipp/scipp

jokasimr · 2026-08-11T13:17:58Z

None is a valid .unit value and we might want to do things like:

def do_something(a, b):
    return a + b.to(unit=a.unit)

But if a.unit is None then .to() will assume the method was called without any argument, and it will raise.

To fix that, make the default unit argument of .to() be a sentinel so that we can clearly distinguish the case when no argument was provided.

SimonHeybrock · 2026-08-14T03:58:05Z

~~Please see discussion in #2468. This is not an oversight to "fix", but was a deliberate choice, since this is semantically different.~~

I think I was misled by both the title and lack of a test: Your intention is that sc.scalar(1,'m').to(unit=None) will raise, right?

SimonHeybrock

Having looked closer (and corrected my earlier comment): this does not enable conversion to/from None -- that is still rejected in C++ (Unit conversion to / from None is not permitted). It only removes the silent no-op for unit=None, which is exactly what #2468 asks for as the alternative. So this enforces that decision rather than overturning it. Agreed on the intent.

One more: to(unit=None, dtype=...) previously meant "dtype only" and now raises. Minor, but it is an API break and might warrant a release note.

SimonHeybrock · 2026-08-14T04:15:57Z

+class _NoUnitProvided:
+    """Sentinel type indicating that no unit argument was passed."""
+
+
+_no_unit_provided = _NoUnitProvided()


The sentinel is defined twice, here and in data_group.py, presumably because operations imports data_group and not the other way round. Please define it once, in a module both can import -- two independent sentinel objects for the same concept is a trap waiting for the first is comparison across module boundaries. Whether that single definition also needs to be publicly exported is the question in my comment on the signature below.

SimonHeybrock · 2026-08-14T04:15:57Z

    var: Variable,
    *,
-    unit: _cpp.Unit | str | None = None,
+    unit: _cpp.Unit | str | None | _NoUnitProvided = _no_unit_provided,


This puts a private type into the public signature and the rendered docs. It also hands the original problem straight to every wrapper of to: downstream code that forwards unit now cannot spell "not provided" without reaching for a private name. DataGroup.to below already has to work around it, and every ess-* wrapper will end up copying that. If we take the sentinel, I think it has to be public and documented.

This is not a real problem. We don't define a global concept for "no argument provided" that is re-used across the project.

This is just a local type to replace None as the default argument. I think sharing that across modules is messy and confusing.

Every ess-* module will not have to copy that because it's not a public interface. to() is not meant to be called without arguments.

SimonHeybrock · 2026-08-14T04:15:57Z

+    if isinstance(unit, _NoUnitProvided) and dtype is None:
        raise ValueError("Must provide dtype or unit or both")

+    if isinstance(unit, _NoUnitProvided):
+        return var.astype(dtype, copy=copy)


isinstance here, is not _no_unit_provided in data_group.py. Use the identity check in both -- that is the point of a sentinel instance, and it keeps the class an implementation detail.

SimonHeybrock · 2026-08-14T04:15:57Z

+        kwargs = {'dtype': dtype, 'copy': copy}
+        if unit is not _no_unit_provided:
+            kwargs['unit'] = unit
+        return self.apply(operator.methodcaller('to', **kwargs))


This kwargs dance is the cost of the sentinel leaking into the signature, made visible. Fine here, but it is the pattern every caller that forwards unit will now have to reproduce -- see my comment on the signature in operations.py.

SimonHeybrock · 2026-08-14T04:15:57Z

+def test_to_with_unit_none() -> None:
+    data = sc.array(dims=["x"], values=[1, 2, 3], dtype="int64", unit=None)
+
+    assert sc.identical(data.to(unit=None), data)


All the new tests cover None -> None, which was already a no-op before this PR (silently, for the wrong reason). The behaviour that actually changes is untested: sc.scalar(1, unit='m').to(unit=None) goes from a silent no-op to UnitError. That is the whole point of the change and the only thing that can regress -- please add it here and for DataArray/DataGroup.

This is a misunderstanding. None -> None was not a no-op before this PR, it was a ValueError that is the issue that the PR addresses.

The example should have been:

var.to(unit=None, dtype='int64')

which used to be a silent no-op (aside from the dtype conversion). But the new behavior is that this raises, right?

I don't see why you think there's something wrong with the example, there seems to be a misunderstanding, let's chat in person.

var.to(unit=None, dtype='int64')

if var has dtype int64 then the above used to be a no-op, but now it will raise and say you can't convert the unit to None.

So this change is breaking.

var.to(dtype='int64')

Will do the same thing before or after this change regardless of the unit of var.

I don't see why you think there's something wrong with the example

You pointed out (correctly) "None -> None was not a no-op before this PR, it was a ValueError that is the issue that the PR addresses.", I looked at my example, "sc.scalar(1, unit='m').to(unit=None) goes from a silent no-op to UnitError." and realized that it indeed does not make sense as written there, i.e., was "wrong" for the behavior-change test that my original comment requested — the behavior change only shows in the presence of a dtype` arg, which my modified code example exercises.

if var has dtype int64 then the above used to be a no-op, but now it will raise and say you can't convert the unit to None.

Yes, that is what I am saying. Where are the tests for that?

What is var here? I can't answer without knowing.

var = sc.scalar(1.0, unit='m') with pytest.raises(sc.UnitError): sc.to_unit(var, unit=None, dtype='float32')

That test raises TypeError: to_unit() got an unexpected keyword argument 'dtype'

Typo, I meant to, not to_unit.

The test has been added 👍

jokasimr · 2026-08-14T09:21:14Z

I think I was misled by both the title and lack of a test: Your intention is that sc.scalar(1,'m').to(unit=None) will raise, right?

Yes that should raise with a UnitError. Before this change it raised with a ValueError.

jokasimr · 2026-08-17T05:31:28Z

I'm not sure the new title is more accurate than before 🤔

We don't raise unconditionally if the unit argument is None, only if the unit of the variable is not None.

fix: allow unit=None in .to()

9bf7494

jokasimr requested a review from nvaytet August 11, 2026 13:19

jokasimr mentioned this pull request Aug 11, 2026

[ESSSPECTROSCOPY] test: add absolute tolerances to make tests less flaky scipp/ess#697

Merged

SimonHeybrock reviewed Aug 14, 2026

View reviewed changes

jokasimr requested a review from SimonHeybrock August 14, 2026 10:11

SimonHeybrock changed the title ~~fix: allow unit=None in .to()~~ Raise UnitError when passing unit=None in .to() Aug 17, 2026

SimonHeybrock changed the title ~~Raise UnitError when passing unit=None in .to()~~ Raise UnitError when passing unit=None in .to() for objects with unit, avoiding silent no-op Aug 17, 2026

jokasimr added 3 commits August 17, 2026 10:39

test: var.to(unit=None) raises if unit of var is not None

d171ed2

Merge branch 'main' into to-allow-none

12c06dc

test: add test

9872b40

SimonHeybrock reviewed Aug 19, 2026

View reviewed changes

Comment thread tests/data_group_test.py Outdated

Apply suggestion from @SimonHeybrock

d3a45f9

SimonHeybrock approved these changes Aug 19, 2026

View reviewed changes

jokasimr merged commit 98fc09a into main Aug 19, 2026
4 checks passed

jokasimr deleted the to-allow-none branch August 19, 2026 07:11

Conversation

jokasimr commented Aug 11, 2026

Uh oh!

SimonHeybrock commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimonHeybrock left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimonHeybrock Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jokasimr Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

jokasimr commented Aug 14, 2026

Uh oh!

jokasimr commented Aug 17, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SimonHeybrock commented Aug 14, 2026 •

edited

Loading

SimonHeybrock left a comment •

edited

Loading

SimonHeybrock Aug 14, 2026 •

edited

Loading

jokasimr Aug 14, 2026 •

edited

Loading

jokasimr Aug 17, 2026 •

edited

Loading

SimonHeybrock Aug 17, 2026 •

edited

Loading

SimonHeybrock Aug 17, 2026 •

edited

Loading