add_particles_with_global_coordinates duplicates particles at np>1 (each rank adds the full array)

[lmoresi]

Summary

Found during #323's gating: when every rank passes the SAME global coordinate array to add_particles_with_global_coordinates() — the natural reading of its docstring — the particles are added once per rank: observed 64 particles from a 32-point array at np2. The docstring implies global/deduplicated semantics.

Impact

read_timestep()-style restoration patterns that pass identical global arrays on all ranks are at duplication risk in parallel. Silent (no error), affects mass/statistics.

Suggested direction

Either claim ownership by locality (each rank adds only the points it owns — kd-tree / cell test — making identical-array calls safe), or document rank-partitioned input semantics loudly and add a guard/assert against full-duplication. Regression test at np2/np4 either way.

Related: 2026-07 audit SWARM findings family; discovered while validating PR #323.

Underworld development team with AI support from Claude Code

[lmoresi]

I don't actually think this is a problem with the routine, it's a problem with the docstring. How can we rule out 1) that this is unintentional and 2) that there is some notion of "identical" for particles that live on the same coordinates.

[lmoresi]

Investigated both questions — the maintainer's instinct is confirmed: the routine is correct, the docstring is the bug — and the false docstring has infected exactly one real caller.

Q1 (intentional?): Yes. Both methods were born together (4841242, 'BASIC swarm'); add_particles_with_global_coordinates is deliberately a low-level primitive with NO locality filtering — its legitimate callers all pass migrate=False and require every rank to keep every supplied point (evaluation swarms at swarm.py:2142, mesh-transfer at discretisation_mesh_variables.py:1350/1390, global evaluation at _function.pyx:496). Its filtering sibling add_particles_with_coordinates (swarm.py:3897) is the one that promises rank-identical-input safety, and delivers it via points_in_domain filtering.

The false sentence is at swarm.py:3986-3990 (added by eb4773f, the #23 'clarify parallel semantics' commit): 'If the same array is passed on every rank, this produces one copy of each point in the correct partition.' Mechanically untrue: migrate() is a scatter (sends non-owned points, keeps owned ones) — it performs no coincident-point dedup, so rank-identical input yields N copies of every point at np=N.

The latent bug the false promise created: Swarm.read_timestep (swarm.py:4218-4238) reads the FULL dataset on EVERY rank and calls the global method with migrate=True → parallel swarm restore produces np-fold duplicated particles. This is the read-everywhere pattern that 9fb198d fixed for SwarmVariable.read_timestep (rank-0 read + routed migration) but never applied to Swarm.read_timestep. Uncaught because test_0003 exercises it serially only, with no count assertion.

Q2 (coincident-particle identity): none exists, and coincidence is harmless-degenerate, never a crash. No dedup anywhere in the swarm/migration code; no particle-ID field is registered (only coords/rank/cellid — PETSc's pid field is unused), so duplicates are indistinguishable except by index. The RBF machinery is epsilon-regularised (weights = 1/(1e-12 + d)^p, normalised — ckdtree.pyx:610-612), so coincident points just dominate/average their own value; nnn=1 proxy updates resolve to one value. The real cost is silent double-counting in integration/statistics — a correctness degradation proportional to the duplication, which is why the np-fold restore duplication matters.

Proposed resolution (small PR): (1) fix the docstring to state the true contract (no dedup; rank-identical input duplicates; correct use = migrate=False full-copy OR pre-partitioned/rank-0 input); (2) fix Swarm.read_timestep to read on rank 0 and route, exactly like the already-correct SwarmVariable.read_timestep; (3) np2 regression test asserting restored particle count. NO locality filter in the primitive itself — that would break the migrate=False callers.

Underworld development team with AI support from Claude Code

[lmoresi]

Fixed by PR #329 (parallel read_timestep restores each particle once, rank-0-routed read; also documents the global-add contract), merged to development on 2026-07-06. Closing manually — auto-close does not fire because PRs target development rather than main.

Underworld development team with AI support from Claude Code