Fix UWexpression parameter display and tensor assignment#49
Open
lmoresi wants to merge 3 commits intodevelopmentfrom
Open
Fix UWexpression parameter display and tensor assignment#49lmoresi wants to merge 3 commits intodevelopmentfrom
lmoresi wants to merge 3 commits intodevelopmentfrom
Conversation
…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
Contributor
There was a problem hiding this comment.
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_rank2where loop never assigned values (always returned zeros) - Added wrapping guard in
_unscaled_matrix_to_rank4to handle UWexpression objects during NDimArray assignment - Added recursive resolution in
_unwrap_atomto 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
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
1 participant
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.
Summary
1e21 Pa.s) instead of symbolic names (e.g.eta_0), and restores lazy evaluation.SympifyError.__getitem__wrapping guard for NDimArray assignment -- the single choke point for all constitutive models aftersimplify()strips per-model wrapping._interpolate_to_proxy()to use internal model-space coordinates for KDTree, avoiding mismatch when units system is active.Test plan
Underworld development team with AI support from Claude Code