Fix unit promotion for same units, different context#845
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #845 +/- ##
=========================================
Coverage ? 90.13%
=========================================
Files ? 22
Lines ? 1713
Branches ? 0
=========================================
Hits ? 1544
Misses ? 169
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
FWIW, this seems reasonable to me, although I'm not especially familiar with the machinery here. |
|
Gentle bump |
|
@sostock any thoughts on this? |
| # same units, but promotion context disagrees | ||
| @inline _promote_unit(x::ContextUnits{N,D,P1,A}, y::ContextUnits{N,D,P2,A}) where {N,D,P1,P2,A} = | ||
| ContextUnits{N,D,promote_unit(P1(), P2()),A}() | ||
| ContextUnits{N,D,typeof(promote_unit(P1(), P2())),A}() |
There was a problem hiding this comment.
I actually don’t see a reason to return ContextUnits here.
When the units and contexts both disagree, we return FreeUnits (line 36 in this file). But when only the contexts disagree, we create new context by promoting the two contexts. This seems inconsistent to me. I think we should either return FreeUnits in both cases or ContextUnits in both cases. I would prefer FreeUnits since making up a new context (that might be different from either of the contexts) seems wrong to me.
The current behavior has been around since ContextUnits were introduced (9545246). But since this particular method errors due to the wrong type parameter, we can change it to whatever we want without breaking something.
Therefore, my preference would be to return FreeUnits here. This can be achieved by just deleting this method (the one at line 36 will be used in its place).
There was a problem hiding this comment.
Actually, on second thought, promoting the contexts might be the right thing to do (since the object should promote as if it had that unit). But then I would argue that the method at line 36 is wrong. However, changing the one at line 36 (to use promote_unit(P1(), P2()) as well) would be breaking.
There was a problem hiding this comment.
For me, this comes up when there's a stray quantity with the "wrong" context. I'd prefer the result become FreeUnits and use the next promotion context it sees rather than gradually contaminate everything with the base unit promotion context. I can see how promoting the contexts could be considered "correct", but I'd be happy with FreeUnits and consistency with the line 36 method.
For the sake of clarity, the _promote_unit method itself doesn't error, but there's an error whenever one tries to use the promotion context, so I think changing this to FreeUnits still doesn't break anything that worked before.
Let me know what you decide, and I can update the PR.
Replace the post-parse `_recontext_length` workaround with a new `DeviceLayout.uparse` that resolves length symbols against `PreferredUnits` directly, so parsed lengths enter the system as `ContextUnits` from the start instead of bare `FreeUnits` that later trip the upstream Unitful mixed-context promotion bug (JuliaPhysics/Unitful.jl#845) inside `layer_z` arithmetic. Non-length symbols (`s`, `Hz`, `fH`, ...) fall back to `Unitful` via try/catch so existing usage is unchanged. Extend the regression test to cover compound expressions (`1/μm`) and the original `level_z` chained-arithmetic failure pattern.
Replace the post-parse `_recontext_length` workaround with a new `DeviceLayout.uparse` that resolves length symbols against `PreferredUnits` directly, so parsed lengths enter the system as `ContextUnits` from the start instead of bare `FreeUnits` that later trip the upstream Unitful mixed-context promotion bug (JuliaPhysics/Unitful.jl#845) inside `layer_z` arithmetic. Non-length symbols (`s`, `Hz`, `fH`, ...) fall back to `Unitful` via try/catch so existing usage is unchanged. Extend the regression test to cover compound expressions (`1/μm`) and the original `level_z` chained-arithmetic failure pattern.
…219) (#220) * Fix YAML-loaded ParameterSet lengths breaking mixed-unit arithmetic (issue #219) `Unitful.uparse` returns `FreeUnits` quantities (promotion target `m`), which mismatches DeviceLayout's `ContextUnits` (target `nm` or `μm`) and broke mixed-unit arithmetic such as `layer_z` in `technologies.jl`. Re-context every length-dimensioned leaf inside `_parse_units!` at parse time so the rest of the package never sees a `FreeUnits` length. `PreferNoUnits` (where `UPREFERRED = NoUnits`) is left as a no-op. * Use PreferredUnits-aware uparse for YAML-loaded ParameterSet values Replace the post-parse `_recontext_length` workaround with a new `DeviceLayout.uparse` that resolves length symbols against `PreferredUnits` directly, so parsed lengths enter the system as `ContextUnits` from the start instead of bare `FreeUnits` that later trip the upstream Unitful mixed-context promotion bug (JuliaPhysics/Unitful.jl#845) inside `layer_z` arithmetic. Non-length symbols (`s`, `Hz`, `fH`, ...) fall back to `Unitful` via try/catch so existing usage is unchanged. Extend the regression test to cover compound expressions (`1/μm`) and the original `level_z` chained-arithmetic failure pattern.
|
I switched to the FreeUnits suggestion above by deleting the buggy method, but can revert if you have a different preference. |
3 participants
Unit promotion for "same units but different promotion context" uses a unit singleton instance as a type parameter, but it should use the type.
Example:
Before the fix:
After the fix: