Skip to content

Fix evaluate() NameError with mixed MeshVariable + coordinate expressions #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lmoresi merged 1 commit into from
Mar 1, 2026
Merged

Fix evaluate() NameError with mixed MeshVariable + coordinate expressions #61

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/underworld3/function/_function.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,27 @@ def _lambdify_and_evaluate(expr, coords, interpolated_results, coord_sys=None, m

r = N.base_scalars()[0:dim]

# 2b. Canonicalize coordinate symbols for lambdify.
# The expression may contain UWCoordinate objects (from mesh.X or
# mesh.CoordinateSystem.unit_e_0) alongside BaseScalar objects. Since
# lambdify uses object identity to map arguments to generated code,
# we must ensure only ONE set of coordinate objects appears.
# Strategy: collect all coordinate-like symbols from the expression,
# group by index, and replace all variants with a single canonical
# sympy.Dummy symbol per coordinate. Use the same Dummy as the
# lambdify argument.
from sympy.vector.scalar import BaseScalar
coord_dummies = [sympy.Dummy(f"_coord_{i}") for i in range(dim)]
coord_subs = {}
for sym in subbedexpr.free_symbols:
if isinstance(sym, BaseScalar):
idx = sym._id[0]
if idx < dim:
coord_subs[sym] = coord_dummies[idx]
Comment on lines +309 to +311

Copilot AI Mar 1, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coordinate canonicalization currently replaces all sympy.vector.scalar.BaseScalar symbols based only on their index (sym._id[0]). This can unintentionally rewrite BaseScalars that belong to a different coordinate system than N (e.g., mesh.Gamma_N.x vs mesh.N.x) and make them evaluate against coords_list silently instead of failing, producing incorrect results. Consider restricting replacements to BaseScalars whose _id[1] matches the CoordSys3D instance N (or otherwise validating the symbol’s coordinate system) before substituting to Dummy.

Suggested change
idx = sym._id[0]
if idx < dim:
coord_subs[sym] = coord_dummies[idx]
# sym._id is typically a tuple (index, CoordSys3D). Only
# canonicalize BaseScalars that belong to the coordinate system N.
try:
coord_idx = sym._id[0]
coord_sys = sym._id[1]
except (AttributeError, IndexError, TypeError):
# If _id is not in the expected form, skip this symbol.
continue
if coord_sys is not N:
# Do not rewrite BaseScalars from other coordinate systems.
continue
if coord_idx < dim:
coord_subs[sym] = coord_dummies[coord_idx]

Copilot uses AI. Check for mistakes.
if coord_subs:
subbedexpr = subbedexpr.xreplace(coord_subs)
r = coord_dummies

Comment on lines +295 to +315

Copilot AI Mar 1, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change fixes a specific mixed MeshVariable + coordinate-system-unit-vector evaluation failure, but there’s no regression test added to ensure it doesn’t reappear. Please add a test that reproduces issue #57 (e.g., evaluating D.sym * mesh.CoordinateSystem.unit_e_0 / unit_e_1 on an Annulus mesh) and asserts the numeric result, so future changes to the lambdify path don’t regress.

Copilot uses AI. Check for mistakes.
# 3. Handle vector/dyadic expressions
if isinstance(subbedexpr, sympy.vector.Vector):
subbedexpr = subbedexpr.to_matrix(N)[0:dim, 0]
Expand Down