SemiLagrangian: remove the unused swarm_degree / swarm_continuous and _workVar (#704) - #705
Conversation
… _workVar (#704) Both parameters were accepted, stored and used only to size a work variable that nothing read: the trace-back samples at psi_star's own nodes and every projection overwrote the work variable's symbol before solving. Sweeping them left the answer bit-identical while the variable spanned 98 to 972 nodes (issue #704). Drop the pair, the allocation, its remesh registration and the docstring/comment that claimed they set the sample points; the projection solver's placeholder source is psi_fn. Denser sampling at the integration points is a separate history manager (PR #703). Fixes #704. Underworld development team with AI support from Claude Code Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G
There was a problem hiding this comment.
🟡 Changes recommended
The updated docstring note should explicitly state that no nodal swarm cache is used (sampling is via uw.function.global_evaluate) to fully eliminate remaining user-facing ambiguity.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR simplifies the SemiLagrangian history manager by removing the unused swarm_degree / swarm_continuous parameters and the associated unused _workVar allocation, aligning the public API and documentation with the implementation (departure-point sampling at the history variable’s own nodes).
Changes:
- Removed
swarm_degree/swarm_continuousfromSemiLagrangian.__init__and deleted the unused_workVarMeshVariable and its remesh registration. - Updated the initial projection-solver placeholder source to use
psi_fndirectly (including the multicomponent/tensor branch). - Adjusted docstrings/comments to stop claiming a separate sampling discretisation is configured via the removed parameters.
File summaries
| File | Description |
|---|---|
| src/underworld3/systems/ddt.py | Drops unused SemiLagrangian sampling kwargs and _workVar, and updates projection placeholder + docs to match the nodal sampling implementation. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| The departure points are the history variable's own nodes, i.e. the | ||
| tracked field's ``degree`` and ``continuous``; there is no separate | ||
| sampling discretisation (the former ``swarm_degree`` / | ||
| ``swarm_continuous`` were never read, issue #704). Denser sampling at | ||
| the integration points is a separate history manager | ||
| (``IntegrationPointSemiLagrangian``, PR #703). |
There was a problem hiding this comment.
Fixed: the class description now says the characteristics are traced from the history variable's own nodes and sampled with uw.function.global_evaluate, and that no swarm is allocated or used.
…history variable's nodes Copilot review on #705: the class description still said 'using nodal swarm'. It now states the trace-back and sampling as implemented. Underworld development team with AI support from Claude Code Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G
…history variable's nodes Copilot review on #705: the class description still said 'using nodal swarm'. It now states the trace-back and sampling as implemented. Underworld development team with AI support from Claude Code Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G
a6377ad to
af72c01
Compare
Fixes #704.
SemiLagrangianacceptedswarm_degree/swarm_continuous, stored them, and used them only to size_workVar, which nothing read: the trace-back samples atpsi_star's own nodes and every projection overwrote the work variable's symbol before solving. The issue's sweep shows the answer bit-identical across the parameters while the variable spanned 98 to 972 nodes.This removes the pair, the allocation, its remesh registration, and the docstring and comment that claimed they set the sample points. The projection solver's placeholder source becomes
psi_fn, which every use already installs before solving. No caller in the tree passed the parameters (one commented-out line in the SLCN solver), no test or doc names them, so they are dropped rather than deprecated. The docstring now says the departure points are the field's own nodes and points to the integration-point history (PR #703) for denser sampling.Semi-Lagrangian test files (snapshot, solver smoke, Navier-Stokes SLCN, recursion, old-frame, units, transport plugin): 59 passed.
Review
MeshVariableperSemiLagrangian(and perAdvDiffusionSLCN, which creates two), so the mesh DM has fewer fields; nothing indexes them by position.TypeError. We chose that over a silent deprecated no-op because a no-op is exactly what the report calls misleading._psi_star_use_multicomponentbranch keeps its placeholder row built frompsi_fnwith the same independent-index selection.Underworld development team with AI support from Claude Code
🤖 Generated with Claude Code
https://claude.ai/code/session_01MSGAFeA7qYXgkuw9ud8F2G