Uh oh!
There was an error while loading. Please reload this page.
This repository was archived by the owner on Aug 20, 2026. It is now read-only.
fix(regularisers): correct diag_bending/diag_all corner cross-term - #81
Merged
Conversation
The boundary-corrected diagonal in `diag_bending` (field 2d/3d, flow 2d/3d) and `diag_all` (flow 2d/3d) expanded each corner weight as w * (fx0*fy0 + fx1*fy0 + fx1*fy0 + fx1*fy1) double-counting `fx1*fy0` and dropping `fx0*fy1`. The correct expansion of (fx0+fx1)*(fy0+fy1) is w * (fx0*fy0 + fx1*fy0 + fx0*fy1 + fx1*fy1) which is exactly the four corner neighbours `matvec_bending` / `matvec_all` already accumulate a few lines above in each of the same files, one-for-one. The error cancels wherever both axes' one-sided boundary signs agree (fx0==fx1 and fy0==fy1 make both forms 4*fx0*fy0), and only manifests where they differ -- which is why the interior-only diagonal tests never caught it. It corrupts the shipped `field_diag`/`flow_diag` output at boundary voxels under sign-flipping boundary conditions (Zero/DST1/DST2) whenever bending is active, and hence any Jacobi preconditioner built from that diagonal. 12 terms across 6 functions in 4 files; a grep for the doubled-term form under regularisers/ now returns nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
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 freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Closes#48
The defect
The boundary-corrected diagonal in
diag_bending(field 2d/3d, flow 2d/3d) anddiag_all(flow 2d/3d) expands each corner weight asfx1*fy0is counted twice andfx0*fy1is dropped. The correct expansion of(fx0+fx1)*(fy0+fy1)isWhy that is the right expansion
Not by assertion — by comparison with the
matvecin the same file.matvec_bendingaccumulates the four corner neighbours explicitly, e.g.regularisers/field/2d.h:383:+ w11 * (get(x0+y0, fx0*fy0) + get(x1+y0, fx1*fy0) + get(x0+y1, fx0*fy1) + get(x1+y1, fx1*fy1))The diagonal's boundary correction is the same four corners folded back onto the centre voxel, so it must carry the same four sign products. Term-for-term the diagonal matched
matvecon the first, second and fourth, and disagreed on the third.field/3d.h:418-423shows the same one-for-one correspondence for all three ofw110/w101/w011.Why it went unnoticed
The error cancels wherever both axes' one-sided boundary signs agree: with
fx0==fx1andfy0==fy1, buggy and correct forms are both4*fx0*fy0. It only bites where the signs differ — e.g.fx0=-1, fx1=+1givesfy0+fy1instead of0. That is a boundary-only, sign-flipping-bound-only effect, so the existing interior-only diagonal checks could never see it.Impact: it corrupts the shipped
field_diag/flow_diagoutput at boundary voxels under sign-flipping boundary conditions (Zero / DST1 / DST2) whenever bending is active, and therefore any Jacobi preconditioner built from that diagonal.Sites
12 terms across 6 functions in 4 files:
regularisers/field/2d.hdiag_bendingregularisers/field/3d.hdiag_bendingregularisers/flow/2d.hdiag_bending,diag_allregularisers/flow/3d.hdiag_bending,diag_allAfter the change, a grep for the doubled-term form under
regularisers/returns nothing:Verification
Full CPU suite (
fastfields-cpu-lib, the project's primary automated gate) built against this branch through the dev-tree symlinks,make -j4 test CXX=clang++— all 13 suites pass, 0 failures:No existing test changes value, which is the expected result: nothing currently on
maincovers the buggy boundary corners. A regression test that does fail without this change is going up separately againstfastfields-cpu-lib— this repo has no test suite of its own — and only passes once this lands and the pin chain reaches it.Only column-aligned expressions changed;
git diff --checkis clean and no whitespace or alignment was disturbed.Provenance
The same defect was independently identified and fixed on the
teenybranch in5ed785a.fastfields-kernels#49was an attempt to port that fix tomain, but it was filed from theclaude-fastfields-to-teenyworkstream, whose PRs must always target theteenybranch — so it was mis-filed rather than wrong. Its analysis is sound and was useful prior art; credit to it. This PR lands the equivalent fix onmainproperly, re-derived and re-verified from scratch against currentmain(mainhas since moved these functions fromstaticmethods usingImpl::qualification onto instance methods holding abound::dyn<b>, so the surrounding code differs).🤖 Generated with Claude Code
https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
Generated by Claude Code