Replace unit handling with Pint OpenwaterHealth/openlifu-python#153

[samueljwu]

Summary

Replaces the unit handling logic in openlifu.util.units with Pint implementation while preserving the legacy API and expected behavior for supported units.

Fixes #153

Changes

• Added pint as a project dependency
• Udpated units.py to use pint.UnitRegistry for deminsionality checks and conversion factors
• Preserved existing functions
• Improved support for units

All existing tests pass
Pre-commit hooks pass

[ebrahimebrahim]

This is wonderful and much needed, thank you!

Adding @peterhollender to review. I can take it as well, @peterhollender let me know

[Copilot]

Pull request overview

This PR replaces custom unit conversion logic in openlifu.util.units with Pint while keeping the existing public helpers used throughout geometry, simulation, and planning code.

Changes:

• Adds Pint as a runtime dependency.
• Reimplements unit normalization, type detection, SI scaling, and conversion through UnitRegistry.
• Expands unit conversion/type tests for legacy units and new Pint-backed aliases.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/openlifu/util/units.py	Replaces manual unit handling with Pint-backed normalization, dimensionality checks, and conversions.
tests/test_units.py	Adds parametrized coverage for conversion factors, unit classification, and newly supported aliases.
pyproject.toml	Adds pint to project dependencies.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ebrahimebrahim]

copilot does have a point here and feel free to come up with a way to address it, but I think it is fine -- no one is currently relying on angle mismatch raising errors or whether or not angles are dimensionless

[ebrahimebrahim]

Thanks for this work, it looks great! Just a couple of changes requested before merge.

(Also I will rebase this branch for you as there has been a lot of change on main since it was put up. So heads up, remember to reset your local)

[ebrahimebrahim]

There is a limitation of how openlifu.util.units works which causes a problem with blind application of pint into it: getunitconversion ultimately returns a multiplier. This works for units that are on a multiplicative scale, but consider what happens for units that aren't:

• what would getunitconversion("degC", "degF") be? F and C are on an affine scale
• what would getunitconversion("dB", "dimensionless") be? dB is on a log scale

We could for now just raise a ValueError for units that are not on a multiplicative scale, to avoid the danger at least

[ebrahimebrahim]

Here we parse from_unit and to_unit repeatedly at every call

I (meaning an AI review agent) ran benchmarks and found that this conversions are 44x slower than the previous implementation.

Since this unit conversion function is called repeatedly per transducer element for some things, we are starting to approach slowdown on the order of 1 second -- starting to get noticeable

Maybe we could cache the conversions?

so put the _quantity(from_unit).to(_normalize_unit(to_unit)).magnitude computation into a helper function and give that caching (e.g. via functools.cache) cached by the (from_unit, to_unit) pair

[samueljwu]

Thanks for checking this out! Summary of the changes:

• Added _is_multiplicative() to reject conversions that cannot be represented by a multiplier
• Added _get_conversion_factor() helper to avoid repeated conversion
• Added a ValueError for angle to non angle conversions

[ebrahimebrahim]

This looks great, thank you!

[ebrahimebrahim]

This is not a mathematically sufficient check which kind of bugs me, but I think you chose a reasonable solution from what was available.

It is acceptable because in Pint (checked 0.24.4 and 0.25.3) there are no unit conversions that preserve 0 but aren't multiplicative. So this check turns out to be sufficient.

The source of truth is pint's own _is_multiplicative metadata whcih suggests the implementation

def _is_multiplicative(unit: str) -> bool:
    return bool(Q_(1, unit)._is_multiplicative)

but that's reaching in and messing with a private attribute _is_multiplicative.

So I think your approach is acceptable here.

[samueljwu]

Thanks for the feedback - I agree. In principle, this check would incorrectly classify a conversion such as y = x^2 as multiplicative. This seems acceptable for now but worth revisiting if there's a better approach available.