fix(darcy): correct transient velocity sign + sign-convention docstrings (#214)#255
Merged
Merged
Conversation
…ngs (#214) The genuine bug: SNES_TransientDarcy.solve projected +darcy_flux for the velocity field, where darcy_flux is the ASSEMBLY flux F = kappa(grad(h)-s). The physical Darcy velocity is v = -F = -kappa(grad(h)-s) (steady SNES_Darcy already used -darcy_flux), so transient runs reported a sign-flipped velocity. Fixed to -darcy_flux to match steady. Docstrings (issue #214): - SNES_Poisson: equation written +div(kappa grad u)=f but the solver assembles -div(kappa grad u)=f -> corrected both docstring instances. - SNES_Darcy / DarcyFlowModel: stated the velocity as +kappa(grad h - s); the code (correctly) returns v = -kappa(grad h - s). Clarified that the model flux property is the assembly flux F and the physical velocity is -F. The assembly flux F1 = kappa(grad(h)-s) is UNCHANGED (it already solves -div(kappa(grad h - s)) = f correctly for all f, including f != 0). Tests (test_1004b): v.grad(h) < 0 (down-gradient) for steady AND transient (the transient assertion fails on the pre-fix +darcy_flux, verified); plus an f != 0 manufactured-solution test locking the source/assembly sign (the existing Darcy tests all use f=0). Closes #214. Supersedes #243 (which flipped the flux instead, introducing an untested f != 0 source-sign regression). Underworld development team with AI support from Claude Code
Member
Author
|
Self-review: minimal sign fix, assembly flux untouched. Transient-velocity regression test verified to fail on the pre-fix code (∫v·∇h flips +0.25). test_1004b 3/3 + existing Darcy 4/4 green in amr-dev. Merging for the release. |
This was referenced Jun 19, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a Darcy sign-convention bug where SNES_TransientDarcy.solve() projected the assembly flux as velocity (sign-flipped physical velocity), aligns transient velocity sign with the steady solver, and updates Darcy/Poisson docstrings and adds regression tests to lock the conventions (including an f != 0 manufactured-solution check).
Changes:
- Fix transient Darcy velocity projection to use
-darcy_flux(physical Darcy velocity) to matchSNES_Darcy. - Correct Poisson and Darcy sign-convention docstrings to match the assembled PDE and returned velocity meaning.
- Add regression tests to detect velocity sign flips and source-term/assembly sign regressions for
f != 0.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/test_1004b_darcy_sign_regression.py |
Adds regression coverage for Darcy velocity sign (steady vs transient) and an f != 0 manufactured solution sign lock. |
src/underworld3/systems/solvers.py |
Fixes transient velocity projection sign; updates Poisson/Darcy docstrings to match solver assembly conventions. |
src/underworld3/constitutive_models.py |
Clarifies DarcyFlowModel.flux as assembly flux and documents physical velocity as -flux. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """int_Omega v . grad(h) dV. For correct Darcy sign (s=0) this is | ||
| int -kappa |grad h|^2 < 0 (flow down-gradient); a flipped velocity gives > 0.""" | ||
| x, y = mesh.X | ||
| grad_h = sympy.Matrix([[p.sym[0].diff(x), p.sym[0].diff(y)]]) |
Comment on lines
+17
to
+19
| import numpy as np | ||
| import sympy | ||
| import pytest |
| - \boldsymbol{s} \right]}_{\mathbf{F}} | ||
| - \nabla \cdot \underbrace{\left[ \boldsymbol{\kappa} \left( | ||
| \nabla h - \boldsymbol{s} \right) \right]}_{\mathbf{F}} | ||
| = \underbrace{W}_{h} |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves the Darcy/Poisson sign-convention issues in #214 with a minimal fix (no change to the assembly flux), superseding #243.
The real bug (behavioural)
SNES_TransientDarcy.solveprojected+darcy_fluxfor the velocity field.darcy_fluxis the assembly fluxF = κ(∇h−s); the physical Darcy velocity isv = −F = −κ(∇h−s). SteadySNES_Darcyalready used−darcy_flux, so transient runs reported a sign-flipped velocity. Fixed to−darcy_fluxto match steady.Docstrings (#214)
+∇·[κ∇u]=fbut the solver assembles−∇·[κ∇u]=f→ corrected both instances.v = +κ(∇h−s); the code correctly returnsv = −κ(∇h−s). Clarified that the constitutivefluxproperty is the assembly fluxFand the physical velocity is−F.What is NOT changed
The assembly flux
F1 = κ(∇h−s)is untouched — it already solves−∇·(κ(∇h−s)) = fcorrectly for allf. (#243 instead flipped the constitutive flux, which fixed the velocity sign but introduced an untestedf≠0source-sign regression — see my review there.)Tests (
tests/test_1004b_darcy_sign_regression.py)v·∇h < 0(flow down-gradient) for steady and transient — the transient assertion fails on the pre-fix+darcy_flux(verified by temporary revert: ∫v·∇h = +0.25).f≠0manufactured-solution test locking the source/assembly sign (existing Darcy tests all usef=0and would not catch a flux-sign flip).Local (amr-dev): test_1004b 3/3, test_1004 3/3, test_1005 1/1 — all green.
Closes #214.
Underworld development team with AI support from Claude Code