Set the PETSc constants on the DS the integrals use, so expression values reach the kernels (#695) - #700
Closed
lmoresi wants to merge 1 commit into
Closed
Set the PETSc constants on the DS the integrals use, so expression values reach the kernels (#695)#700lmoresi wants to merge 1 commit into
lmoresi wants to merge 1 commit into
Conversation
…lues reach the kernels (#695) uw.maths.Integral, BdIntegral and CellWiseIntegral compile their integrands through the same JIT as the solvers, which routes every uw.function.expression to PETSc's constants array, but none of them ever called PetscDSSetConstants: the kernels read zeros, so any integrand with a viscosity, a time or another expression in it integrated to nothing, and a fresh Integral returned the same zero from the cache. Found on the DFG cylinder drag, where the viscous traction (eta is an expression) vanished and the drag read 23 to 28% low on two meshes without moving with the SUPG weights. Each class now packs the manifest and sets the constants right after the objective; the boundary integral sets them on its sandbox DS, which has its own discrete system. Regression test test_0503 covers the three classes, a changed value without recompilation, and the constitutive-flux traction that found it. 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_018T2VHUGaZiQVJ95qQ4DiSL (cherry picked from commit 5131039)
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The fix is targeted, consistent with existing solver constant-update patterns, and is backed by a focused regression test covering the previously broken cases.
Pull request overview
This PR fixes a correctness bug in Underworld3’s FEM integral evaluation path where uw.function.expression constants embedded in an integrand were compiled into PETSc DS constants but never actually set on the PetscDS used for evaluation—causing those constants to be read as zero inside the kernel (notably breaking traction/flux-based integrals such as cylinder drag, #695).
Changes:
- Add helpers in
petsc_maths.pyxto pack the JIT constants manifest and callPetscDSSetConstants()on thePetscDSused byIntegralandCellWiseIntegral. - Ensure
BdIntegralsets constants on the sandbox DM’s DS (since the sandbox has its own DS created viaDMCreateDS). - Add a level_1/tier_a regression test covering volume, boundary, and cell-wise integrals, including constant updates without recompilation and a constitutive-flux boundary integral.
File summaries
| File | Description |
|---|---|
src/underworld3/cython/petsc_maths.pyx |
Packs and applies JIT expression constants to the correct PetscDS before integral evaluation (including the boundary sandbox DS). |
tests/test_0503_integral_expression_constants.py |
Adds regression coverage to ensure expression constants reach integral kernels across integral types and after value updates. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
Contained in #688, merged to development as 1d5e8c1: the same 🤖 Generated with Claude Code |
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.
Integral,CellWiseIntegralandBdIntegralcompile their integrand through the JIT with a constants manifest but never calledPetscDSSetConstantson the DS they evaluate on, so everyuw.function.expressionin an integrand was read as zero by the kernel. A viscous traction integral over a cylinder therefore returned the pressure drag alone (the 'drag deficit' on the Navier-Stokes benchmarks, #695). The fix packs the manifest and sets it on the DS (the sandbox DS for boundary integrals) after the objective is set.tests/test_0503_integral_expression_constants.pycovers a volume, boundary and cell-wise integral of an expression constant, the value changing afterc.sym = ..., and a constitutive-model flux on a boundary.Extracted from #688 as a bug fix in its own right. Closes #695.
🤖 Generated with Claude Code
https://claude.ai/code/session_018T2VHUGaZiQVJ95qQ4DiSL