Skip to content

Materials: a registry and two distributions, and a JIT constant-collision fix - #715

Merged
lmoresi merged 11 commits into
developmentfrom
feature/particle-demos
Sep 11, 2026
Merged

Materials: a registry and two distributions, and a JIT constant-collision fix#715
lmoresi merged 11 commits into
developmentfrom
feature/particle-demos

Conversation

@lmoresi

@lmoresi lmoresi commented Sep 9, 2026

Copy link
Copy Markdown
Member

Started as two particle demonstrations; ended up as a material system and a silent correctness bug in the JIT.

The bug first

Now split out as #717, a bugfix/ branch off development, since it is a correctness fix independent of everything else here. The commit below (d53d0038) is the same content; the branches merge cleanly except for tests/test_0103_jit_rampable_constants.py, where this branch's end-to-end test uses the MaterialSwarm API introduced here — take this branch's version of that file.

A layered viscosity built from two ViscousFlowModels returned the uniform-viscosity answer: L2 2.819e-1 against the exact layered Couette profile, and 1.502e-10 against plain linear shear. The composed flux was correct and so was the constants manifest — (0, \eta = 1.0), (1, \eta = 1000.0) — and only the emitted C was wrong.

_JITConstant is a plain sympy.Symbol subclass that never adopted the disambiguation UWexpression uses (docs/developer/design/SYMBOL_DISAMBIGUATION_2025-12.md), and it had neither half of it. It constructed through Symbol.__new__, which is cached by name:

a, b = _JITConstant(0, name='same'), _JITConstant(1, name='same')
a is b                    # True
a._ccodestr, b._ccodestr  # constants[1], constants[1]

The second placeholder was the first object, and assigning its _ccodestr overwrote the first one's. Every occurrence then rendered as one constants[] slot, so the blend collapsed to (phi_0 + phi_1) * constants[k]. Every ViscousFlowModel calls its viscosity \eta, so any model with two of them was affected, not just multi-material.

Fixed structurally: construct via Symbol.__xnew__ to bypass the cache, and put the slot index in _hashable_content, so identity is the slot rather than the name. The same solve now gives 1.807e-7. Separately, the manifest sort tie-broke on str(expr) — a UWexpression's current value — so two same-named constants could swap slots when a parameter changed; it now tie-breaks on instance_number.

Regression tests at both levels in test_0103_jit_rampable_constants.py.

Materials

A model script should not write a level set, a mask, or a blend.

materials = uw.swarm.MaterialSwarm(mesh, fill_param=3)
materials.add("mantle", shear_viscosity_0=1.0,   density=3300)
materials.add("slab",   shear_viscosity_0=1.0e3, density=3400)
materials["slab"] = mesh.X[1] > 0.53

stokes.constitutive_model = uw.constitutive_models.ViscousFlowModel
stokes.materials = materials
stokes.bodyforce = -materials.density * mesh.CoordinateSystem.unit_e_1

solver.materials sets every constitutive-model parameter the materials declare and the model recognises, by name. One the model does not own (density) is not pushed anywhere; it is a blended symbol for the script to use where it belongs.

What a material is and where it is are kept apart — which is what the existing materials register was reaching for. MaterialRegistry holds the definitions: mesh-free, shareable, exportable, and its values may be numbers, quantities, or laws (eta_0 * sympy.exp(-T.sym[0])). Two distributions consume one registry:

MaterialSwarm MaterialRegions
material moves yes no
level sets sampled from particles exact, 0 or 1
population control needed none
cost per step a proxy fill nothing, built once

MaterialRegions takes a gmsh physical group, a mesh label, a (label, value) pair, a geometric condition resolved at the integration points, a boolean array, or a callable.

The partition of unity earns its place precisely when a property is a law: there is then no number to store at an integration point, and the only way to combine N expressions into one symbol the assembler can compile is the weighted sum.

MaterialRegistry keeps create_material / set_property / get_property / export_config / import_config / add_callback and the create_standard_* helpers. Removed: assign_to_region and evaluate_property_field, which returned numpy from region IDs and could never reach a weak form — MaterialRegions is that idea, finished. IndexSwarmVariable stays as the machinery and stays importable, but is no longer the documented route.

A declared property that no model recognises and nothing reads is reported at solve time only when it is a close match to a parameter the model does have: viscocity is flagged against viscosity, density is silent.

proxy_sampling

proxy_location says where a swarm variable's proxy lives; the new proxy_sampling says what each point reads.

  • "reconstruct" — a weighted fit over the nnn nearest particles, linear-exact. What a smooth field wants (the default).
  • "share" — the mean over the particles this point speaks for: those whose nearest integration point within their own cell is this one. The cell-restricted Voronoi share.

The share is what a history wants — stress, damage, accumulated strain — because it is bounded by the particle values, so it cannot invent a stress the swarm never held, while the reconstruction overshoots a discontinuity and gets worse as particles are added:

particles/cell reconstruct range share range
3 −0.051 … +1.137 0.000 … 1.000
8 −0.068 … +1.108 0.000 … 1.000
15 −0.109 … +1.097 0.000 … 1.000

There is deliberately no "nearest" for a plain SwarmVariable: sampling one particle's value whole is the material mapping, and putting the property field on the particles hands the solver an answer where it needs a constitutive law. Asking for it raises, and the message says so.

IndexSwarmVariable now defaults to proxy_location="integration_points". Nodes measured worst everywhere (layered Couette 8.0e-2 against 1.8e-7) and its smear is about one cell wide however many particles are added — a property of the basis, not of the swarm.

At a cut-cell interface, nearest sampling converges to a floor set by the rule at ~8 particles per cell (3.48e-2 → 1.85e-2 → 1.85e-2 at fill 3/8/15); past that, refine the mesh. The share gives fractional masks there, and then the mixing rule matters: createMask is arithmetic (Voigt) and does not converge (3.53e-2 flat), while the same masks blended harmonically (Reuss) do (1.83e-2 → 1.24e-2). materials.mixing(shear_viscosity_0="harmonic") selects it.

Locating particles is the expensive half of any cell-local operation (18.8 ms for 32,912 particles against 3.2 ms for the share itself), so Swarm._owning_cells() caches it and repopulate's census reads the same cache. Lagrangian_Swarm forwards proxy_sampling, so a viscoelastic stress history can ride on the share.

Repopulation keeps a label a label

Swarm.repopulate gave new particles a reconstruction of their neighbours' values, which is wrong for an integer material index: the average of two labels is not a label. Integer variables now take their nearest neighbour's value whole; nearest= names any other variable to treat the same way.

Demonstrations and docs

docs/advanced/particle-population-and-materials.md with figures, and runnable scripts in docs/examples/utilities/intermediate/: population control in an extending box, and the layered material problem. docs/api/materials.md and docs/developer/subsystems/integration-point-variables.md updated.

Tests

test_0071 (16), test_0072_material_swarm.py (13), test_0073_material_regions.py (7), test_0103 (+2). Full level_1 and tier_a suite: 1223 passed, 3 skipped, 1 xfailed.

Underworld development team with AI support from Claude Code

🤖 Generated with Claude Code

https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G

…a label; two demonstrations

IndexSwarmVariable gains proxy_location, so a material index can be read
where the assembler looks. "integration_points": every integration point
takes the material of its NEAREST PARTICLE, so each level set is exactly 0
or 1, the interface keeps its sub-cell position and nothing can overshoot
(the Ellipsis / Underworld particle-in-cell material mapping). "cells": a
polynomial material fraction per cell, clamped to [0, 1] and renormalised
so the masks still sum to one. "nodes" is unchanged and remains the
default.

Measured on two viscosity layers (1 and 1000) with the interface on mesh
edges, where the exact velocity lies in the P2 space so the only error is
the material representation: the nodal level set ramps across a whole cell
and leaves an L2 velocity error of 8.0e-2; at the integration points the
same solve is exact to solver tolerance, 1.8e-7. All three integrate the
viscosity correctly in the mean (500.5000), which is why a bulk diagnostic
does not see the difference.

Swarm.repopulate: an INTEGER variable now takes its nearest neighbour's
value whole instead of a reconstruction, since the average of two material
labels is not a label; `nearest=` names other variables to treat the same
way. Float fields keep their linear-exact reconstruction.

Two demonstrations, with figures, in a new user-facing page
docs/advanced/particle-population-and-materials.md and runnable scripts in
docs/examples/utilities/intermediate/: population control in an extending
box (without it the box drains to 13% of its particles and 626 of 764
cells are empty; with it, none are), and the material index above.

Also removes a dead docstring block left in IndexSwarmVariable.__init__.

Underworld development team with AI support from Claude Code

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G
Copilot AI lite review requested due to automatic review settings September 9, 2026 20:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

There are a few correctness/documentation issues in the new code paths (starved-rank guard for integration-point mapping, nearest= handling for string inputs, and broken/incorrect doc references) that should be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR extends UW3’s particle material workflow by (1) letting IndexSwarmVariable project material masks at integration points or as per-cell fractions, and (2) ensuring Swarm.repopulate() preserves integer “labels” via nearest-neighbour copying. It also adds two runnable demonstrations and a focused test suite to validate boundedness, partition-of-unity, and accuracy for a layered-viscosity benchmark.

Changes:

  • Add proxy_location={"nodes","integration_points","cells"} to IndexSwarmVariable, including direct particle→integration-point mapping and a per-cell polynomial fraction path.
  • Add nearest= support to Swarm.repopulate() and automatically treat integer-valued swarm variables as “nearest-copy” during repopulation.
  • Add advanced docs, two intermediate examples, and a new level-1/tier-a test module covering the new behaviors.
File summaries
File Description
src/underworld3/swarm.py Implements new IndexSwarmVariable.proxy_location behaviors and label-preserving repopulation logic.
tests/test_0071_index_swarm_proxy_location.py Adds tests for boundedness/partition-of-unity, integration-point exactness, layered Couette accuracy, repopulation label preservation, and population control behavior.
docs/examples/utilities/intermediate/Ex_Swarm_Population_Control.py Demonstrates population control in a pure-shear extending box.
docs/examples/utilities/intermediate/Ex_Swarm_Material_Index.py Demonstrates layered-viscosity accuracy improvements from integration-point material mapping.
docs/advanced/particle-population-and-materials.md New advanced documentation page explaining population control and material mapping, with figures and links to examples.
docs/advanced/index.md Links the new advanced documentation page from the advanced docs index.
Review details

Suppressed comments (1)

docs/advanced/particle-population-and-materials.md:116

  • The documentation points to docs/examples/utilities/Ex_Swarm_Material_Index.py, but the example added in this PR lives under docs/examples/utilities/intermediate/. This broken path will confuse readers and makes the doc page harder to follow.
`docs/examples/utilities/Ex_Swarm_Material_Index.py`. Two viscosity layers,
1 and 1000, carried as a material index and driven from the top. With the
interface on mesh edges the exact velocity is piecewise linear and lies in the
P2 velocity space, so the only error in the solve is how the material is
  • Files reviewed: 6/9 changed files
  • Comments generated: 4
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/underworld3/swarm.py
Comment thread src/underworld3/swarm.py
Comment thread docs/advanced/particle-population-and-materials.md Outdated
Comment thread docs/examples/utilities/intermediate/Ex_Swarm_Population_Control.py Outdated
lmoresi and others added 7 commits September 9, 2026 14:47
…ur review findings

The base SwarmVariable constructor sets _proxy_location from its own default
and runs AFTER the subclass assignment, so an IndexSwarmVariable built with
proxy_location="integration_points" stored "nodes" and the new fill never
dispatched: the level sets were the right kind of variable, filled by the
nodal inverse-distance algorithm. Assign after super().__init__.

Re-measured with the dispatch live, the layered Couette test changes for the
better: with the interface on mesh edges every particle in a cell is on one
side, so the per-cell fit is constant and "cells" is exact too (1.8e-7, was
4.9e-2 through the nodal algorithm). The distinction shows in the cut-cell
case, now documented: only the integration-point mapping improves with
particle density (3.5e-2 to 1.9e-2 from 10 to 55 particles per cell, against
8.4e-2 to 7.7e-2 for nodes and 4.1e-2 flat for cells).

Copilot's findings, all fixed: the starved-rank guard was `local_size <= 1`,
but one particle is a well-defined nearest-particle answer, so only a rank
with none is starved; `nearest="M"` iterated the characters of the string;
two documented example paths were missing the intermediate/ directory; and
an example comment credited `nearest=` for what happens automatically for
integer variables.

Underworld development team with AI support from Claude Code

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G
…apability test

First class means the particle field's symbol goes where a mesh variable's
symbol goes. Measured, for all three proxy targets: arithmetic with mesh
variables and sympy, uw.maths.Integral, uw.function.evaluate anywhere,
projection onto a mesh variable, and a solver term (viscosity and body force
at once) all work. The one exception is a gradient of the integration-point
form, which is refused rather than answered wrongly: the element that holds
a value at each integration point has no gradient to give.

The page now says that first, since the choice of proxy changes where the
sampling lands and not what you can write; the accuracy tables stay as the
reason to prefer one target.

Underworld development team with AI support from Claude Code

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G
…and measure it

Louis asked whether the gradient could come from a projection through the
integration points. It can, and the machinery already computes it: the
"cells" proxy IS that projection, a least-squares polynomial per cell, so it
differentiates directly and with no global solve.

Measured, recovering d/dx of a quadratic particle field: cells degree 2
2.4e-7, nodes degree 2 1.1e-4, an explicit global L2 projection onto P2 then
differentiate 1.1e-4, cells degree 1 1.3e-3, nodes degree 1 2.1e-3. The
per-cell fit wins because it is local and exact for polynomials up to its
degree.

The integration-point form still refuses a derivative, since its own
tabulated gradient is identically zero and answering would be a silent zero,
but the message now names the remedy instead of leaving the reader to find
it. Documented with the table, and tested.

Underworld development team with AI support from Claude Code

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G
…eak form still refuses

Louis: "Evaluate is not the same path as the residual calculation, after
all." Right, and the guard was treating them alike. They are now
deliberately different, and both say so:

- A weak form still refuses a derivative of an integration-point variable.
  Answering would mean either the silent zero its own tabulation gives, or a
  reconstruction chosen behind the user's back and paid for at every
  assembly. Which discretisation the gradient comes from is a modelling
  decision, so it stays the user's: proxy_location="cells" gives level sets
  that are already polynomials.
- uw.function.evaluate is a query and now answers, through
  _integration_point_sources_to_cell_fit: any integration-point source under
  a derivative is replaced by a per-cell least-squares fit of its own values
  (cached on the mesh like the other evaluate work variables), and the
  ordinary derivative machinery differentiates that. The fit is allowed to be
  exactly determined, since the rule is unisolvent for the degree; the
  default nmin would send every cell to the linear patch and leave the
  recovered gradient first order.

Measured on a quadratic particle field: the recovered gradient converges,
2.4e-3 / 6.1e-4 / 2.6e-4 as the cell size halves from 1/5 to 1/20. The direct
"cells" route reaches 2.4e-7, exact for a quadratic with nothing projected
afterwards, so the docs point a solve there and call evaluate's answer a
diagnostic.

Stated in the user page, the developer subsystem page, the
IntegrationPointVariable docstring, both SwarmVariable proxy_location
docstrings, and the refusal message itself. Tested on both paths.

Underworld development team with AI support from Claude Code

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G
A layered viscosity built from two ViscousFlowModels returned the
UNIFORM-viscosity answer: L2 2.819e-1 against the exact layered Couette
profile, and 1.502e-10 against plain linear shear. The composed flux was
correct and so was the constants manifest ((0, \eta = 1.0),
(1, \eta = 1000.0)); only the emitted C was wrong.

_JITConstant is a plain sympy.Symbol subclass that never adopted the
disambiguation UWexpression uses (docs/developer/design/
SYMBOL_DISAMBIGUATION_2025-12.md), and it has neither half of it. It
constructed through Symbol.__new__, which is cached by NAME, so the
second placeholder was literally the first object and assigning its
_ccodestr overwrote the first one's:

    a, b = _JITConstant(0, name='same'), _JITConstant(1, name='same')
    a is b                    -> True
    a._ccodestr, b._ccodestr  -> constants[1], constants[1]

Every occurrence then rendered as one constants[] slot, so the blend
collapsed to (phi_0 + phi_1) * constants[k]. Every ViscousFlowModel
calls its viscosity \eta, so any model with two of them was affected,
not just multi-material.

Construct via Symbol.__xnew__ to bypass the cache and put the slot index
in _hashable_content, so identity is the slot rather than the name. The
same solve now gives 1.807e-7 against the layered exact solution.

Also: the manifest sort tie-broke on str(expr), which for a UWexpression
is its current VALUE, so two same-named constants could swap slots when
a parameter changed. Tie-break on instance_number instead.

Regression tests at both levels in test_0103_jit_rampable_constants.py.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G
proxy_location says WHERE a swarm variable's proxy lives; the new
proxy_sampling says WHAT each point reads. The two are orthogonal and
were previously conflated in one reconstruction.

  reconstruct  a weighted fit over the nnn nearest particles, linear
               exact - what a smooth field wants (the default)
  share        the mean over the particles this point speaks for: those
               whose nearest integration point WITHIN THEIR OWN CELL is
               this one - the cell-restricted Voronoi share

The share (utilities/particle_share.py) is the closest a fixed
quadrature rule gets to true Voronoi integration: the rule points
partition their own cell, every particle lands in exactly one part, and
nothing crosses a cell wall. That is what a HISTORY wants - stress,
damage, accumulated strain - because the value is bounded by the
particle values (it is a mean of them) so it cannot invent a stress the
swarm never held, while the reconstruction overshoots a discontinuity
and gets WORSE as particles are added:

  particles/cell   reconstruct range    share range
  3                -0.051 .. +1.137     0.000 .. 1.000
  8                -0.068 .. +1.108     0.000 .. 1.000
  15               -0.109 .. +1.097     0.000 .. 1.000

There is deliberately no "nearest" for a plain SwarmVariable. Sampling
one particle's value whole is the MATERIAL mapping, and a material is
an IndexSwarmVariable; putting the property field on the particles and
sampling it hands the solver an answer where it needs a constitutive
law. Asking for it raises, and the message says so.

IndexSwarmVariable now defaults to proxy_location="integration_points"
with proxy_sampling="nearest". Nodes measured worst everywhere (layered
Couette 8.0e-2 against 1.8e-7) and its smear is about one cell wide
however many particles are added - a property of the basis, not of the
swarm. update_type only means anything at the nodes and now warns
elsewhere; test_0115 asks for nodes explicitly.

At a cut-cell interface, nearest sampling converges to a floor set by
the rule at ~8 particles per cell (3.48e-2 -> 1.85e-2 -> 1.85e-2 at
fill 3/8/15); past that, refine the mesh. The share gives fractional
masks there, and then the answer depends on the mixing rule: createMask
is arithmetic (Voigt) and does NOT converge (3.53e-2 flat), while the
same masks blended harmonically (Reuss) do (1.83e-2 -> 1.24e-2).

Locating particles is the expensive half of any cell-local operation
(18.8 ms for 32,912 particles against 3.2 ms for the share itself), so
Swarm._owning_cells() caches it, drops it wherever _kdtree is dropped,
and repopulate's census now reads the same cache.

Lagrangian_Swarm forwards proxy_sampling, so a viscoelastic stress
history can ride on the share.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G
A model script should not write a level set, a mask, or a blend. It
should name its materials and their properties, say where each one is,
and hand the whole thing to the solver:

    materials = uw.swarm.MaterialSwarm(mesh, fill_param=3)
    materials.add("mantle", shear_viscosity_0=1.0,   density=3300)
    materials.add("slab",   shear_viscosity_0=1.0e3, density=3400)
    materials["slab"] = mesh.X[1] > 0.53

    stokes.constitutive_model = uw.constitutive_models.ViscousFlowModel
    stokes.materials = materials
    stokes.bodyforce = -materials.density * mesh.CoordinateSystem.unit_e_1

solver.materials sets every constitutive-model parameter the materials
declare AND the model recognises, by name. One the model does not own
(density) is not pushed anywhere; it is a blended symbol for the script
to use where it belongs. Properties can change and regions be repainted
afterwards - the blend is symbolic and the push repeats.

WHAT a material is and WHERE it is are kept apart, which is what the
materials register was always reaching for. MaterialRegistry holds the
definitions: mesh-free, shareable between models, exportable as plain
data, and its property values may be numbers, quantities, or LAWS
(eta_0 * sympy.exp(-T.sym[0])). Two distributions consume one registry:

  MaterialSwarm     carried by particles, so the material advects
  MaterialRegions   tied to the mesh - a gmsh physical group, a mesh
                    label, or a geometric condition resolved at the
                    integration points. Exact, no particles, no
                    population control, built once.

Both build the same partition of unity. It earns its place precisely
when a property is a law: there is then no number to store at an
integration point, and the only way to combine N expressions into one
symbol the assembler can compile is the weighted sum.

MaterialRegistry keeps create_material / set_property / get_property /
export_config / import_config / add_callback and the create_standard_*
helpers, with values now symbolic and property names the ones a
constitutive model knows (MaterialProperty.VISCOSITY is "viscosity",
already the alias for shear_viscosity_0). Removed: assign_to_region and
evaluate_property_field, which returned numpy from region IDs and could
never reach a weak form - MaterialRegions is that idea, finished.

A declared property that no model recognises and nothing reads is
reported at solve time only when it is a CLOSE MATCH to a parameter the
model does have: "viscocity" is flagged against "viscosity", "density"
is silent. A misspelled viscosity is otherwise silently the default one.

IndexSwarmVariable stays as the machinery and stays importable; it is no
longer the documented way to do materials.

Layered Couette through the new interface: L2 1.807e-7, assembled
int(eta) 500.5000 exactly - the same as the raw route. Verified on a VEP
model too, with shear_viscosity_0 and shear_modulus both pushed and the
stress history on the share.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G
@lmoresi lmoresi changed the title Material index at the integration points, and two particle demonstrations Materials: a registry and two distributions, and a JIT constant-collision fix Sep 10, 2026
@lmoresi
lmoresi requested a lite review from Copilot September 10, 2026 18:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

proxy_sampling="share" is currently blocked by the local_size <= 1 guard in _rbf_to_meshVar, which can leave stale integration-point proxies on ranks holding exactly one particle.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 22/26 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/underworld3/swarm.py
Comment on lines 1527 to 1532
Values = current_values
elif getattr(self, "_proxy_sampling", "reconstruct") == "share":
Values = self._share_to_integration_points(
meshVar, self.unpack_raw_data_from_petsc(squeeze=False)
)
elif monotone:
@lmoresi

lmoresi commented Sep 10, 2026

Copy link
Copy Markdown
Member Author

Review from the 2026-09 style/clutter audit. Findings against the Style Charter, scoped to lines this PR adds.

1. swarm.py:2953 — a self-assignment whose comment claims something it does not do.

for var in self._meshLevelSetVars:
    var.data[:, 0] = var.data[:, 0]          # keep, but write collectively

Reading a local array and writing the same values back is not a collective. If
the starved-rank path genuinely needs one — and this is the empty-rank class we
have been chasing for months — then this line is silently skipping it, and the
level-set variables on a starved rank are left in whatever state the previous
step put them in while the other ranks move on. If no collective is needed, the
statement is dead and the comment is wrong. Either way it cannot stay as
written (§4: a comment states the constraint the code cannot show; §5: no dead
code).

Worth saying which it is in the PR: the answer decides whether this is a tidy-up
or a correctness fix.

2. swarm.py:2947uw.mpi.rank in a user-visible warning.

f"IndexSwarmVariable proxy update: rank {uw.mpi.rank} holds "

§11: the user should never see parallel/MPI calls; there is a wrapper and it
works better. uw.pprint / uw.selective_ranks exist for exactly this.

3. Flat .data where §7 mandates .arraymaterials.py:609, 611, 644;
swarm.py:2978, 2987, 2996; swarm_materials.py:214;
_function.pyx:851. These compute or write external values, so they are not
the one sanctioned flat-.data use (a raw variable-to-variable copy inside the
non-dimensionalisation boundary). Filed as #722 with the merged instances,
since the pattern is wider than this PR.

4. The tests this PR adds sit in the test_006* band, which no glob in
scripts/test.sh reaches — see #721. They have not run in CI.

Underworld development team with AI support from Claude Code

lmoresi and others added 2 commits September 10, 2026 15:10
Adversarial review of the new material API found nine confirmed defects plus
two ergonomic traps. Every fix below has a regression test that reproduces the
defect against the previous revision.

**A mistyped label value segfaulted every rank.** getStratumIS() for a value
outside a label's live set HARD-ABORTS PETSc -- no exception, no traceback --
and the guard `if stratum is not None` never fired because petsc4py returns a
live IS wrapping a NULL handle. The API's own error message steered users
straight in ("say which with materials[name] = (label, value)"), and on a UW3
box the only valid value is 99999, so every guess crashed. The repo documents
this hazard twice, in its own words, and I had not looked. Now: probe
getValueIS() first, guard on size, and destroy the IS (it was leaked).

**Label values are rank-local.** A label live on one rank and absent on
another raised on some ranks and not others -- a hang. Worse, a two-valued
label looked single-valued to each rank separately, so a model that failed
LOUDLY in serial silently painted the union of both zones at np=2. The value
set is now reduced across ranks before anything branches on it: identical
behaviour at np=1/2/3, and an explicit value gives the same area (0.3401) at
every rank count.

The rest, each confirmed and each now refused or fixed:

- two distributions with the same explicit name silently SHARED their level
  sets, and painting the second changed the first one's answers; the only
  diagnostic was a print to stdout. Detected by counting what the mesh
  actually gained -- predicting the sanitised variable names is wrong,
  "M^{[0]}" becomes "M0".
- delete_material / registry.add after a build re-pointed the blend at the
  wrong level sets, or produced a material with none (an IndexError swallowed
  into a warning while the solver kept a stale blend). The registry now
  refuses structural change once allocated.
- hasattr(materials, prop) RAISED KeyError, breaking the contract for every
  caller, and hasattr also triggered the collective build.
- a property sharing a name with a real attribute was silently unreachable,
  and the two distributions have different attributes, so a name that worked
  on one was shadowed on the other. Warned at add().
- m["x"] = A; m["x"] = B gave the UNION. Assignment now replaces.
- harmonic mixing with a zero value folded to ComplexInfinity and died in the
  C printer naming neither material nor property; with a vanishing law it gave
  Integral = nan behind a RuntimeWarning. Refused, naming the material.
- mixing() for an undeclared property was accepted silently -- the one place a
  typo could have been caught for free.
- units were non-dimensionalised at add() time, so the same declaration meant
  numbers 28 orders of magnitude apart depending on whether the model's
  reference quantities were set yet. Resolved on READ instead, which is what
  lets a registry be written before the model.
- solver.materials = None cleared the solver's reference but left the
  distribution pushing to it.

check()'s message claimed a parameter "keeps its default value" when it was in
fact being set from the materials that do declare it, and repeated on every
rebuild. Fixed both; the similarity false positives on *_1/*_max/*_0 names are
inherent to the threshold and remain.

The documentation page raised if followed top to bottom -- stokes.materials =
allocated the particles, so the SwarmVariable in the next block came too late.
Reordered, and verified by running every block verbatim. The collective
contract on the first read is now stated, with a public materials.build() to
fix the ordering where it matters.

Full level_1 and tier_a: 1230 passed. Material tests 43/43 at np=1 and np=2.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G
Style-charter findings from the adversarial review.

Section 3 bans naming a variable `model` (it is ambiguous between uw.Model and
a constitutive model); materials.py did it twice, in the two places where it
IS the constitutive model.

Section 7: new code uses `.array` with the three-index shapes, and flat
`.data` has exactly one sanctioned exception. The nine uses in the new library
source were none of them that exception; converted.

Section 11 and section 1: both new examples were written on `os.environ` for
options, `np.savez` for output, bare per-rank `print`, `mesh.dm` calls, and
private reads (`swarm._particle_coordinates`, `mesh._robust_owning_cells`) --
and the material-index one documented the environment-variable idiom as the
way to run it. Both are rebuilt on uw.Params (with -uw_name CLI overrides),
uw.pprint, and public API only. Their diagnostics are now GLOBAL integrals
rather than rank-local norms and censuses, so they run and report correctly on
more than one rank: verified identical at np=1 and np=2.

The population-control example changed substance, not just style. Its old
claim rested on a particle scatter plot. Measuring instead what the field
actually does, on the layer's own area against the exact e^-t thinning:

    proxy_location        population control    layer area vs exact
    cells                 on                    1.03x
    cells                 off                   5.03x
    integration_points    on                    1.06x
    integration_points    off                   1.07x

Three diagnostics I tried first did NOT discriminate between control on and
off, because the nearest-particle mapping degrades gracefully -- an empty cell
still finds a plausible particle. Starvation wrecks the per-cell fit, which
needs particles IN that cell, and that is the case the example now shows. The
docs page carries the table too, since it previously asserted the effect
without a field-level number.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G
@lmoresi

lmoresi commented Sep 10, 2026

Copy link
Copy Markdown
Member Author

Adversarial review round. Nine confirmed defects plus two ergonomic traps, all fixed with regression tests that reproduce them against the previous revision.

The worst one killed the job. getStratumIS() for a value outside a label's live set hard-aborts PETSc — no exception, no traceback, every rank gone — and the guard if stratum is not None never fired, because petsc4py returns a live IS wrapping a NULL handle. The API's own error message steered users straight in ("say which with materials[name] = (label, value)"), and on a UW3 box the only valid value is 99999, so every guess crashed. The repo documents this hazard twice, in its own words; I had not looked.

Label values are rank-local, which made a two-valued label fail loudly in serial and silently paint the union of both zones at np=2. The value set is now reduced across ranks before anything branches on it — identical behaviour at np=1/2/3.

The rest: two distributions sharing a name silently shared level sets (detected by counting what the mesh gained, not by predicting sanitised names); delete_material/registry.add after a build re-pointed the blend; hasattr raised KeyError and triggered the collective build; a property sharing a name with a real attribute was silently unreachable; m["x"] = A; m["x"] = B gave the union; harmonic mixing divided by a zero value; mixing() accepted undeclared names; units were resolved at add() so the same declaration meant numbers 28 orders apart; solver.materials = None kept pushing.

Style charter: model renamed (§3), the library source moved to .array (§7), and both examples rebuilt on uw.Params/uw.pprint with public API and global diagnostics (§11/§1) — they now run correctly on more than one rank.

The population-control example changed substance. Its claim rested on a scatter plot; three field-level diagnostics I tried first did not discriminate at all, because the nearest-particle mapping degrades gracefully. Starvation wrecks the per-cell fit:

proxy_location population control layer area vs exact
cells on 1.03x
cells off 5.03x
integration_points on 1.06x
integration_points off 1.07x

So the honest claim is narrower: population control is cheap and always safe, but "cells" is where it is necessary.

Full level_1 and tier_a: 1230 passed. Material tests 43/43 at np=1 and np=2.

Two things left for a maintainer decision, both noted in the thread: the .data/.array conflict between the charter, CLAUDE.md and the CI gate's own remediation string; and whether the new tests should enter as tier_b rather than tier_a.

Underworld development team with AI support from Claude Code

…sal message

Three conflicts, all from this branch still carrying d53d003 -- the original
version of the JIT constant-collision fix -- after that work was split out to
PR #717 and diverged there. Resolved to development's version in every case:

- _jitextension.py and test_0103: development has both halves of the fix
  (the slot index in the NAME for ordering, and in _hashable_content for
  identity) plus the three regression tests. This branch had only the first
  attempt.
- constitutive_models.py: development carries the shortened note the charter
  review asked for, not the incident narrative.

Taking --theirs wholesale for _jitextension.py also discarded an unrelated
change this branch has in the same file: the integration-point derivative
refusal message, which points at proxy_location='cells' and says evaluate()
answers the same query. test_0071 caught it. Restored -- minus the benchmark
figure it quoted, which belongs in the subsystem docs rather than in a runtime
error (charter section 4).

Full level_1 and tier_a on the merged tree: 1245 passed, 3 skipped, 1 xfailed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G
@lmoresi
lmoresi merged commit 224d65a into development Sep 11, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants