projection: unit-aware smoothing_length (supersedes #200)#234
Merged
Conversation
…pping Reimplements the good parts of #200 on top of current development (which already grew an equivalent smoothing_length API), so it lands cleanly instead of a 145-commit reconcile-rebase. For the scalar, vector and multi-component projectors (tensor inherits the scalar): - a dimensional input (Pint Quantity / UnitAware) is non-dimensionalised and reduced to a plain float — uw.non_dimensionalise() returns a dimensionless UWQuantity which sympify() cannot square (the bug flagged on #200); - negative lengths raise ValueError instead of silently storing nonsense; - _smoothing_is_dimensional is tracked so the getter round-trips a dimensional input as a Pint Quantity and a plain input as a plain float. Adds tests/test_0505_projection_smoothing_length.py (tier_a/level_1, 8 tests) locking the round-trip on all four projector classes, the Pint-quantity path, the negative-raises contract, and the screened-Poisson low-pass behaviour. Supersedes #200. Underworld development team with AI support from Claude Code
5 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines the smoothing_length API across the projection solver classes by making unit-aware inputs reliably round-trip, adding validation for negative values, and aligning the stored representation with SymPy expectations (store as plain float before sympify()).
Changes:
- Update
smoothing_lengthgetter/setter logic to properly handle dimensional inputs viauw.non_dimensionalise(...).magnitudeand reject negative lengths. - Track whether
smoothing_lengthwas set dimensionally (_smoothing_is_dimensional) to control whether the getter returns a Pint quantity vs a plain float. - Add a new test module locking round-trip behavior, unit handling under scaling, negative-input validation, and a basic end-to-end low-pass smoothing behavior check.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/underworld3/systems/solvers.py |
Implements unit-aware smoothing_length behavior changes (float extraction, negative validation, dimensional round-trip tracking). |
tests/test_0505_projection_smoothing_length.py |
Adds coverage for smoothing_length round-tripping, unit-aware scaling behavior, validation, and basic smoothing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2493
to
+2499
| # Return a Pint Quantity only if the user set a dimensional value via | ||
| # the setter; plain-float input round-trips as a plain float so the | ||
| # meaning of the number the user passed is preserved. | ||
| if getattr(self, "_smoothing_is_dimensional", False): | ||
| return uw.scaling.dimensionalise( | ||
| L_nd, uw.scaling.units.meter) | ||
| except Exception: | ||
| return L_nd | ||
| return L_nd |
Comment on lines
+2516
to
2525
| is_dim = hasattr(L, "magnitude") or hasattr(L, "units") | ||
| if is_dim: | ||
| L_nd = uw.non_dimensionalise(L) | ||
| except Exception: | ||
| # Fall back to magnitude-or-float coercion if the | ||
| # value doesn't carry/expect units. | ||
| if hasattr(L, "magnitude"): | ||
| L_nd = L.magnitude | ||
| else: | ||
| L_nd = L | ||
| L_nd = float(getattr(L_nd, "magnitude", L_nd)) | ||
| else: | ||
| L_nd = float(L) | ||
| if L_nd < 0: | ||
| raise ValueError(f"smoothing_length must be ≥ 0, got {L_nd}") | ||
| self._smoothing_is_dimensional = is_dim | ||
| self._smoothing = sympify(L_nd) ** 2 |
Comment on lines
+2734
to
2743
| is_dim = hasattr(L, "magnitude") or hasattr(L, "units") | ||
| if is_dim: | ||
| L_nd = uw.non_dimensionalise(L) | ||
| except Exception: | ||
| if hasattr(L, "magnitude"): | ||
| L_nd = L.magnitude | ||
| else: | ||
| L_nd = L | ||
| L_nd = float(getattr(L_nd, "magnitude", L_nd)) | ||
| else: | ||
| L_nd = float(L) | ||
| if L_nd < 0: | ||
| raise ValueError(f"smoothing_length must be ≥ 0, got {L_nd}") | ||
| self._smoothing_is_dimensional = is_dim | ||
| self._smoothing = sympify(L_nd) ** 2 |
Comment on lines
+3086
to
3095
| is_dim = hasattr(L, "magnitude") or hasattr(L, "units") | ||
| if is_dim: | ||
| L_nd = uw.non_dimensionalise(L) | ||
| except Exception: | ||
| if hasattr(L, "magnitude"): | ||
| L_nd = L.magnitude | ||
| else: | ||
| L_nd = L | ||
| L_nd = float(getattr(L_nd, "magnitude", L_nd)) | ||
| else: | ||
| L_nd = float(L) | ||
| if L_nd < 0: | ||
| raise ValueError(f"smoothing_length must be ≥ 0, got {L_nd}") | ||
| self._smoothing_is_dimensional = is_dim | ||
| self._smoothing = sympify(L_nd) ** 2 |
Comment on lines
+101
to
+123
| model = uw.Model() | ||
| model.set_reference_quantities( | ||
| length=uw.quantity(1000, "m"), | ||
| nondimensional_scaling=True, | ||
| ) | ||
| uw.use_nondimensional_scaling(True) | ||
| try: | ||
| V = uw.discretisation.MeshVariable( | ||
| "V_pint", mesh, vtype=uw.VarType.SCALAR, degree=2, continuous=True) | ||
| proj = sys.Projection(mesh, V) | ||
| proj.smoothing_length = uw.quantity(0.05, "m") | ||
|
|
||
| # Non-dim store: 0.05 m / 1000 m = 5e-5; α = 2.5e-9. | ||
| assert float(proj.smoothing) == pytest.approx(2.5e-9) | ||
|
|
||
| # Pint-quantity round-trip. | ||
| L_out = proj.smoothing_length | ||
| assert hasattr(L_out, "magnitude"), \ | ||
| f"Pint input should round-trip as a Pint Quantity, got {type(L_out)}" | ||
| assert float(L_out.to("m").magnitude) == pytest.approx(0.05) | ||
| finally: | ||
| uw.use_nondimensional_scaling(False) | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Reimplements the good parts of #200 on top of current development. Development independently grew an equivalent
smoothing_lengthAPI since #200 branched, so #200 became a 145-commit reconcile-rebase with conflicts insolvers.py. This branch ports only the genuine improvements onto development's base, so it lands cleanly.Changes
For the scalar, vector and multi-component projectors (tensor inherits the scalar):
uw.non_dimensionalise()returns a dimensionlessUWQuantity, whichsympify()cannot square; extracting.magnitudefixes it.smoothing_lengthraisesValueErrorinstead of silently storing nonsense (and the getter no longer returnsNone)._smoothing_is_dimensionalis tracked so a dimensional input reads back as a Pint Quantity in metres, while a plain input reads back as a plain float.Tests
tests/test_0505_projection_smoothing_length.py(tier_a / level_1, 8 tests, all passing locally): round-trip on all four projector classes, the α-view consistency, the Pint-quantity round-trip under an active scaling context, the negative-raises contract, and the screened-Poisson low-pass behaviour.Supersedes #200 — that PR can be closed once this is reviewed.
Underworld development team with AI support from Claude Code