Skip to content

Fix for NS solver - #83

Closed
bknight1 wants to merge 2 commits into
developmentfrom
fix/BK-NS_solver
Closed

Fix for NS solver#83
bknight1 wants to merge 2 commits into
developmentfrom
fix/BK-NS_solver

Conversation

@bknight1

Copy link
Copy Markdown
Member

Fix for the solver ##82

  • Change from SL to Eulerian history manager for flux & unknown in NS solver.
  • Does NOT fix the SL issue. Further investigation required.
  • Does NOT fix the callback error in the Eulerian history manager.

Fix for the solver ##82
- Change from SL to Eulerian history manager for flux & unknown in NS solver.
- Does NOT fix the SL issue. Further investigation required.
- Does NOT fix the callback error in the Eulerian history manager.
@bknight1
bknight1 requested a review from lmoresi as a code owner March 18, 2026 07:21
Copilot AI review requested due to automatic review settings March 18, 2026 07:21

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.

Pull request overview

Updates the Navier–Stokes solver’s default history-term managers, moving away from always constructing Semi-Lagrangian operators and toward Eulerian (mesh-based) time-derivative tracking.

Changes:

  • Replace default DuDt construction with an Eulerian_DDt-based implementation (including grid-based advection correction via V_fn).
  • Only construct DFDt when not supplied by the caller, and default it to an Eulerian history manager.
  • Comment out the prior Semi-Lagrangian constructor blocks in-place.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread src/underworld3/systems/solvers.py
Comment thread src/underworld3/systems/solvers.py
Comment thread src/underworld3/systems/solvers.py Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@lmoresi

lmoresi commented Mar 19, 2026

Copy link
Copy Markdown
Member

@bknight1 — Thanks for the reproducer script and diagnosis. We've identified and fixed the root cause on development (commit 4d522d8), so this workaround PR is no longer needed.

Root cause: The SemiLagrangian DDt's update_pre_solve computed upstream departure points without clamping them to the domain boundary. Near corners (especially the lid-driven cavity singularities), departure points landed outside the mesh. global_evaluate then extrapolated using the P2 basis, producing values ~1000x the actual field maximum. This injected a massive spurious acceleration that blew up the solver on step 2.

Fix: Two lines — clamp mid_pt_coords and end_pt_coords using mesh.return_coords_to_bounds() in SemiLagrangian.update_pre_solve(). The Lagrangian DDt already did this for swarm advection; the SemiLagrangian (nodal) path was missing it.

Closing this PR in favour of the direct fix on development. We'd appreciate it if you could validate the fix and do further benchmarking — including testing the second-order DuDt term. See issue #82 for details and benchmark results.

Underworld development team with AI support from Claude Code

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.

3 participants