Fix UWexpression parameter display and tensor assignment by lmoresi · Pull Request #49 · underworldcode/underworld3

lmoresi · 2026-02-18T02:46:40Z

Summary

ExpressionDescriptor.set: Store UWexpression as symbolic reference instead of extracting inner value. Fixes parameter display showing numeric values (e.g. 1e21 Pa.s) instead of symbolic names (e.g. eta_0), and restores lazy evaluation.
_unwrap_atom: Recursively resolve UWQuantity inside UWexpression chains during JIT compilation, preventing SympifyError.
_unscaled_matrix_to_rank4: Add __getitem__ wrapping guard for NDimArray assignment -- the single choke point for all constitutive models after simplify() strips per-model wrapping.
_unscaled_matrix_to_rank2: Fix pre-existing bug where loop body never assigned values (always returned zeros).
get_units() in unit_conversion.py: Restore delegating wrapper for compiled Cython modules (ckdtree.pyx) that import from this location.
Surface coordinate space handling: Add dimensional/ND gateways to Surface class. Fix _interpolate_to_proxy() to use internal model-space coordinates for KDTree, avoiding mismatch when units system is active.

Test plan

ViscousFlowModel with UWexpression(UWQuantity) -- solve succeeds
TransverseIsotropicFlowModel with UWexpression(UWQuantity) -- solve succeeds
Plain numeric and plain UWexpression parameters -- no regression
Stokes solver tests (test_1010) -- 11/11 passing
Poisson solver tests (test_1000, test_1001) -- 9/9 passing
Rank-2 tensor roundtrip (mandel_to_rank2 fix) -- 2D and 3D verified
Surface _interpolate_to_proxy with units/scaling active -- KDTree coordinate match verified

Underworld development team with AI support from Claude Code

…ive models ExpressionDescriptor.__set__ was extracting the inner value from UWexpression assignments (value._sym), losing the symbolic reference. Parameters displayed as "1e21 Pa.s" instead of the symbolic name (η₀), and lazy evaluation broke. Now stores the UWexpression itself as a symbolic reference, preserving display, lazy evaluation, and the unit delegation chain. Two downstream issues were also fixed: - _unwrap_atom: recursively resolve UWQuantity when found inside UWexpression chain, preventing SympifyError during JIT compilation - _unscaled_matrix_to_rank4: wrap values with __getitem__ (UWexpression from MathematicalMixin) before NDimArray assignment, matching the existing guard in _build_c_tensor. This is the single choke point for all constitutive models after simplify() strips the per-model wrapping. Also fixed pre-existing bug in _unscaled_matrix_to_rank2 where the loop body never assigned values to the output matrix (always returned zeros). Underworld development team with AI support from Claude Code

Copilot

Pull request overview

This PR fixes critical issues with parameter handling and tensor assignment in Underworld3's symbolic expression system. It addresses bugs that prevented proper display of symbolic parameter names, broke lazy evaluation chains, and caused tensor conversion functions to fail or return incorrect values.

Changes:

Simplified UWexpression parameter assignment to preserve symbolic references and lazy evaluation
Fixed critical bug in _unscaled_matrix_to_rank2 where loop never assigned values (always returned zeros)
Added wrapping guard in _unscaled_matrix_to_rank4 to handle UWexpression objects during NDimArray assignment
Added recursive resolution in _unwrap_atom to handle UWQuantity nested inside UWexpression chains

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
src/underworld3/utilities/_api_tools.py	Simplified ExpressionDescriptor.set to store UWexpression as symbolic reference instead of extracting inner value, preserving lazy evaluation and symbolic display
src/underworld3/maths/tensors.py	Fixed _unscaled_matrix_to_rank2 bug where loop body never assigned values; added getitem wrapping guard in _unscaled_matrix_to_rank4 for NDimArray assignment
src/underworld3/function/expressions.py	Added recursive UWQuantity resolution in _unwrap_atom to prevent SympifyError during JIT compilation

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

The function was removed during a cleanup but ckdtree.pyx (compiled Cython) still imports it. Added a thin delegate to units.get_units(). Underworld development team with AI support from Claude Code

Surface stores geometry internally in model (ND) space but user-facing properties (vertices, control_points) must return dimensional coordinates when the units system is active. Changes: - Add _dimensionalise_coords() gateway for Surface output properties - Add set_control_points() input gateway that non-dimensionalises - Fix _interpolate_to_proxy() to use internal ND coordinates for KDTree (was using the now-dimensional .vertices property, causing mismatch with mesh._coords) Underworld development team with AI support from Claude Code

lmoresi · 2026-02-24T04:59:44Z

Open review period ended, ready to merge.

lmoresi · 2026-02-24T05:03:37Z

These are important bug fixes to the code rather than feature upgrades. This will be merged once the tests are fixed.

lmoresi · 2026-02-24T05:08:17Z

CI failure is pre-existing and unrelated to this PR: 7 tests marked xfail now pass (xpassed), which pytest treats as failure in strict mode. The xfail markers on those tests are stale — the bugs they expected have been fixed. These markers should be removed in a follow-up so CI goes green.

The Fatal Python abort is also pre-existing (PyVista/VTK cleanup in headless CI).

All 280 actual tests pass. Merging.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

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

Copilot · 2026-02-24T05:09:03Z

+                    points = np.asarray(points.to_base_units().magnitude) / cs._length_scale
+                else:


In set_control_points(), the units-handling path assumes any unit-bearing input without .to_base_units() can safely use .magnitude directly. UnitAwareArray does not implement to_base_units(), so a UnitAwareArray in (for example) km would be treated as if it were already in base SI meters before dividing by cs._length_scale, producing incorrect internal model coordinates. Consider explicitly converting UnitAwareArray via its .to('meter') (or by constructing a Pint quantity with points.units and calling .to_base_units()) before stripping magnitude.

Suggested change

points = np.asarray(points.to_base_units().magnitude) / cs._length_scale

else:

points = np.asarray(points.to_base_units().magnitude) / cs._length_scale

elif hasattr(points, "to"):

# Handle UnitAwareArray and similar types that provide .to() but

# not .to_base_units(). Explicitly convert to meters first.

points_in_m = points.to("meter")

magnitude = getattr(points_in_m, "magnitude", points_in_m)

points = np.asarray(magnitude) / cs._length_scale

else:

# Fallback: assume magnitude is already in base units

Copilot · 2026-02-24T05:09:03Z

        if coords is None:
-            coords = mesh.X.coords
-
-        if hasattr(coords, 'magnitude'):
+            coords = np.asarray(mesh._coords)
+        elif hasattr(coords, 'magnitude'):
            coords = coords.magnitude
        elif hasattr(coords, '__array__'):


SurfaceCollection.transfer_normals() now treats coords as model-space coordinates (matching mesh._coords / surface.face_centers). If callers pass unit-bearing coordinates (Pint Quantity / UnitAwareArray), the current logic strips .magnitude but does not convert from physical units back to model coordinates when the mesh coordinate system is scaled. This will query the KDTree in the wrong coordinate space when units/scaling are active. Consider mirroring set_control_points() by dividing by mesh.CoordinateSystem._length_scale (after converting to base SI) when _scaled is true.

These tests were marked xfail for bugs that have since been fixed (primarily by PR #49). Pytest treats unexpected passes (xpass) as failures in strict mode, causing CI to report failure despite all 280 tests actually passing. Tests promoted: - test_0720: RBF mesh variable state pollution resolved - test_0750: UnitAwareExpression .units, multiplication closure, get_units on compound expressions — all fixed - test_0753: UWexpression ND scaling now works - test_0757: UWexpression multiplication with coordinates fixed Underworld development team with AI support from Claude Code

Copilot AI review requested due to automatic review settings February 18, 2026 02:46

Copilot started reviewing on behalf of lmoresi February 18, 2026 02:47 View session

Copilot AI reviewed Feb 18, 2026

View reviewed changes

lmoresi added 2 commits February 19, 2026 14:46

Restore get_units() in unit_conversion.py for ckdtree compatibility

f664320

The function was removed during a cleanup but ckdtree.pyx (compiled Cython) still imports it. Added a thin delegate to units.get_units(). Underworld development team with AI support from Claude Code

lmoresi requested a review from Copilot February 24, 2026 05:01

lmoresi self-assigned this Feb 24, 2026

Copilot started reviewing on behalf of lmoresi February 24, 2026 05:02 View session

Copilot AI reviewed Feb 24, 2026

View reviewed changes

lmoresi merged commit 5eeaaaf into development Feb 24, 2026
4 of 5 checks passed

lmoresi deleted the fix/constitutive-model-uwexpression-params branch June 13, 2026 00:53

Navigation Menu

Uh oh!

Fix UWexpression parameter display and tensor assignment - #49

Fix UWexpression parameter display and tensor assignment#49
lmoresi merged 3 commits into
developmentfrom
fix/constitutive-model-uwexpression-params

lmoresi commented Feb 18, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

lmoresi commented Feb 24, 2026

Uh oh!

lmoresi commented Feb 24, 2026

Uh oh!

lmoresi commented Feb 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		points = np.asarray(points.to_base_units().magnitude) / cs._length_scale
		else:

-                    points = np.asarray(points.to_base_units().magnitude) / cs._length_scale
-                else:
+                    points = np.asarray(points.to_base_units().magnitude) / cs._length_scale
+                elif hasattr(points, "to"):
+                    # Handle UnitAwareArray and similar types that provide .to() but
+                    # not .to_base_units(). Explicitly convert to meters first.
+                    points_in_m = points.to("meter")
+                    magnitude = getattr(points_in_m, "magnitude", points_in_m)
+                    points = np.asarray(magnitude) / cs._length_scale
+                else:
+                    # Fallback: assume magnitude is already in base units

Uh oh!

Conversation

lmoresi commented Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

lmoresi commented Feb 24, 2026

Uh oh!

lmoresi commented Feb 24, 2026

Uh oh!

lmoresi commented Feb 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lmoresi commented Feb 18, 2026 •

edited

Loading