|
FhSim
3.1.0
Marine systems simulation
|
|
|
FhSim
3.1.0
Marine systems simulation
|
| ID | 0004 |
| Class | KNOWN-LIMITATION |
| Severity | 1 |
| Status | blocked |
| Models | Net/NetStructure |
| Found | 2026-09-15 audit (e3f3107) |
| Decision needed | How to fulfil the intent of the panel-Jacobian test, given that its finite-difference reference is untrustworthy at the current fixture (see the 2026-09-16 investigation): fix the checker methodology, redesign the fixture, or accept the limitation and correct the stale comment. A latent ~9x accuracy error in the internal panel block is masked by matched-step pinning and may warrant a separate BUG. |
src/net/NetStructure.cpp:1185
and src/net/NetStructure.cpp:1198-1211, which perturbs each of the 18 panel inputs by h = eps * std::max(1.0, std::abs(inputs[grp][k])), re-evaluates NetElement3N::AddNodeForces, and divides the difference by h — a one-sided forward difference, not a closed form.
The test pins the checker's step to the same value, tests/NetStructure_Jacobian_Test.cpp:34:
with the reasoning in the comment immediately above it (tests/NetStructure_Jacobian_Test.cpp:16-22 and :30-33).
This was recorded as a known numerical property so the next reader would not "tidy up" the perturbationEps line. The 2026-09-16 investigation (below) shows the situation is worse than that framing: the matched-step pin does not just avoid a spurious mismatch, it masks a ~9x error in the internal panel Jacobian. Two things follow:
SimObject.NetStructure_PanelJacobian compares two one-sided forward differences taken with the same step formula, so their truncation errors cancel and the test passes whether or not the derivative is correct. It is near-tautological, not a verification.absTol at any step.The intent of SimObject.NetStructure_PanelJacobian is to prove the panel block of OdeJacobian is consistent with the panel force derivative in OdeFcn. The 2026-09-16 investigation (below) shows the current test does not prove that, and that no closed form can be verified at the current fixture. Options for fulfilling the intent:
tests/NetStructure_Jacobian_Test.cpp:16-33) to state the true default-step magnitudes (~30, not "below 3") and that the pass is a matched-step artefact, not a verified derivative. Cheapest; leaves the ~9x internal error in place, documented.fhsim core package, so this is a cross-repository change.absTol, then either keep the FD panel block (now genuinely verified) or derive the closed form and verify against a central difference. This is a fixture + methodology change, not a NetStructure.cpp edit.NetElement3N::AddNodeForces and drop the perturbationEps override. Recorded for completeness, but the investigation shows this cannot be verified at the current fixture (even an analytically-correct block fails the checker at its default step, maxAbsErr=30.8), and the force law is ~250 lines and deeply piecewise, so it is high-risk without (b) or (c) in place first.Recommendation: (b) then (d) is the only path that ends with a genuinely verified analytic Jacobian; (c) is the in-repo alternative if the checker cannot be changed. If neither is wanted now, do (a) so the record stops claiming the test verifies something it does not — and file the latent accuracy error (see below) as its own finding so it is not lost.
which intent is being bought? If the panel Jacobian only needs to be good enough for implicit-integrator convergence, the current forward difference may be acceptable once the ~9x error is understood — but the test must then stop advertising a verification it does not perform (a). If it needs to be verifiably correct, the checker's forward-difference reference has to become central/Richardson (b) or the fixture has to move to low curvature (c) before any closed form (d) can be trusted.
Under (b)/(c): the panel block agrees with a central-difference (or Richardson) reference to absTol at the checker's default step, with the perturbationEps override removed. Under (a): no code test — the change is to the comment and, ideally, a new issue for the accuracy error.
Option (a) leaves a ~9x-wrong internal Jacobian in place; it is masked today because the checker shares its truncation error, and it only affects implicit-integrator step size and convergence, not output values directly — but it is a real latent error, not a cosmetic one. Options (b)/(c)/(d) each rewrite either the reference or the derivative of the panel force law; any error is silent under an implicit integrator, so none may be merged without the checker passing against a trustworthy (central/Richardson) reference at the default step.
This is a note rather than a defect: the panel block of NetStructure::OdeJacobian is a forward-difference approximation where an analytic block would be expected. Replacing it with a closed-form Jacobian is real numerics work that changes convergence behaviour of implicit integrators and needs its own verification against the finite-difference result. It is out of scope for a risk-free fix pass. Smallest owner decision: whether an analytic panel Jacobian is wanted for 3.x at all.
An attempt at option (d) (derive the closed form, drop the perturbationEps override, pass the checker at its default step) was made and reverted with no code change, because the investigation showed the premise is unsound at the current fixture. Empirical results, Release build:
JacobianChecker.cpp) and the internal panel block (ComputePanelJacobianFD, NetStructure.cpp:1198-1211) use the same one-sided forward-difference step formula eps*max(1,|x|). With both at eps=1e-7 their truncation errors are identical and cancel, so the test passes regardless of whether either value is correct.maxAbsErr bottoms out around ~3 near step 1e-9 (still >> absTol=0.1) and grows in both directions (30.8 at 1e-8; ~70 at 1e-10; ~529 at 1e-11): truncation-limited at large steps, cancellation-limited at small steps, with a noise floor above tolerance.maxAbsErr=30.8 at checker step 1e-8. So no analytically-correct panel Jacobian can pass this fixture's checker as configured.tests/NetStructure_Jacobian_Test.cpp:16-33 claims the default-step mismatches are "every one below 3 in magnitude"; at the current fixture they reach absErr ~30. The comment predates the fixture's node-spacing change.NetElement3N::AddNodeForces is a ~250-line, deeply piecewise function (rotation from three points, Priour tension with kinks, Reynolds-dependent drag via a 7th-order log-polynomial plus a separate piecewise Ersdal-Faltinsen branch, asin/acos with domain clamps, saturation bounds, knot drag, damping) that calls external sfh:: helpers, so a hand-derived closed form is high-risk and, per the verification gate, must not be merged without a trustworthy reference to check it against.
Latent accuracy error worth its own finding. Point 2 is a real, currently-masked defect: the internal panel Jacobian is ~9x off at the fixture states, hidden only by the matched-step pinning. A central-difference internal block corrects it (34.7->3.83) but breaks the currently-pinned test because it no longer shares the checker's truncation error. This should be filed as a separate BUG (masked-accuracy) if the owner does not take (b)/(c) now.
Reviewed against the code and confirmed by a second pass. Claims below are grounded in ComputePanelJacobianFD (src/net/NetStructure.cpp:1156-1220) and the core checker NumericalJacobian (fhsim conan package, src/testtools/JacobianChecker.cpp).
We want a Jacobian: the slope of the panel force with respect to each of its 18 inputs. Today that slope is estimated by a one-sided forward difference — nudge the input up by a small step h, see how the force changes, divide by h:
This estimate carries an error proportional to ‘h * f’'(the curvature). The panel force is extremely curved at the current fixture (f'' ~ 6e8`), so the forward-difference slope is **~9x too big** there: 34.70 where the true slope is ~3.83.
The test that is supposed to catch this uses the same one-sided forward difference with the same step as its "reference". Two estimates that are wrong in the same way agree with each other, so the test passes while both are wrong. It is a near-tautology, not a check. The pinned line perturbationEps = 1e-7 is what forces the two steps to match.
A central difference nudges the input both up and down and averages:
Its error is proportional to ‘h^2 * f’'', which is far smaller than the forward difference'sh * f''`. That is why the central-difference slope (3.8325) is stable across steps while the forward one (34.70) is not.
The implicit integrator uses this Jacobian as a modified-Newton iteration matrix, and it matters exactly how. In Euler1imp.cpp (fhsim core):
OdeJacobian is formed once per timestep, before the Newton loop (Euler1imp.cpp:97,115), and reused across every Newton iteration in that step.G(x) = x - x_n - dt*f(t+dt, x) using the true OdeFcn, not the Jacobian.So the converged answer is set by the true residual and is correct no matter how wrong the Jacobian is. A poor Jacobian cannot corrupt outputs; it can only cost extra Newton iterations or rejected steps. This is the key point behind the owner's argument: the panel Jacobian's accuracy is not a goal in itself — the goal is the fastest time-domain simulation, and Jacobian accuracy only matters insofar as it reduces total solver work.
That gives a genuine accuracy-vs-performance trade for the model block:
| Panel-block variant | Cost to form J (per panel, per step) | Effect on Newton convergence |
|---|---|---|
| One-sided forward (today) | 18 AddNodeForces + 1 base | Can be ~9x off at high curvature → potentially more iterations / rejected steps |
| Central difference | 36 AddNodeForces + 1 base | Accurate → potentially fewer iterations |
| Analytic closed form | 1 evaluation, no perturbation loop | Accurate and cheapest to form |
Which variant makes the whole simulation fastest is not decidable by inspection — it depends on how often each variant's inaccuracy triggers extra iterations/rejections versus the per-step cost of forming J, which in turn depends on panel count and step size. It has to be measured.
Step 1 — make the checker accurate first (unconditional, zero runtime cost). Change the core checker's NumericalJacobian from one-sided forward to central difference or Richardson extrapolation, and remove the test's perturbationEps = 1e-7 pin. The checker runs only in tests, so more evaluations there cost nothing at simulation time, and there is no accuracy-vs-speed trade to weigh — it should simply be as accurate as possible. This is issue option **(b)**. It gives a trustworthy oracle that can measure how far any model-Jacobian variant is from the truth, and it is the prerequisite for trusting anything below. JacobianChecker lives in the **fhsim core conan package**, so this is a cross-repository change. *(If the current fixture's curvature makes even a central reference uncomfortable, also apply option (c) — move the fixture to a lower-curvature state — so the oracle has margin.)*
Step 2 — measure whether model-Jacobian accuracy actually buys simulation speed. With an accurate oracle in place, instrument a representative net simulation and compare the current forward-difference panel block against a central-difference block on the metrics that decide runtime: total wall-clock time, Newton iteration counts, and rejected/retried steps. Two outcomes:
Step 3 — if accuracy pays off, pick the cheapest accurate block.
AddNodeForces is ~250 lines and deeply piecewise: high-risk to hand-derive, and per this issue's verification gate it must not be merged without the accurate Step-1 oracle to check it against.Primary recommendation: (1) accurate checker + pin removed → (2) profile forward vs central in a real simulation → (3) adopt the analytic closed form only if measurement shows Jacobian quality moves wall-clock time, else keep the cheapest block that converges. Never merge a changed model block without the accurate Step-1 oracle passing at the default step — a wrong implicit Jacobian is silent in the outputs and shows up only as slower convergence.
How often the integrator re-forms the Jacobian (freeze-and-reuse across several steps) is a separate performance knob owned by the integrator, not by this model block. If simulation speed is the true objective, it is worth checking there too — but it is out of scope for NetStructure.cpp and this issue.
Done in fhsim core (editable working tree /home/karlr/_work/_DEV/fhsim, branch main):
src/testtools/JacobianChecker.cpp, NumericalJacobian: changed the reference from one-sided forward (f(x+h) - f(x))/h to central difference (f(x+h) - f(x-h))/(2h). Truncation error drops from ‘O(h*f’')toO(h^2*f'''), so the reference is now trustworthy at the net-panel's high curvature. This is a test-only path, so the extra evaluation per column costs nothing at simulation time. -include/fhsim/testtools/JacobianChecker.h: updated theperturbationEpsdoc to name the central-difference quotient.TheperturbationEpsdefault (1e-8) is unchanged — the investigation showed central difference is stable to 4 sig figs across1e-4...1e-8` even at the hard fixture.Verified: rebuilt fhsim_test + testFhVis and ran the full Jacobian/integrator surface (JacobianChecker.*, ImplicitIntegrationSim.*, *JacobianTriggers*, *JacobianSparsity*, TestToolsUnit.*, ModelAssemblyService.*, SparseComposedLink.*, LinearSolverSelection.*, *DirectionalDeriv*): 103 passed, 0 failed. The exact-analytic models (VanDerPol, Heat1D, chains) still pass against the more accurate reference, as expected. clang-format clean.
Not done here, and why (scope of "do #1"):
fhsim_marine_elements consumes fhsim as a conan package, so this checker change does not reach it until fhsim is rebuilt, repackaged, and the version re-pinned here. That package bump is the delivery step for Step 1 on this side.perturbationEps = 1e-7 pin in tests/NetStructure_Jacobian_Test.cpp was not removed. Removing it would make SimObject.NetStructure_PanelJacobian legitimately fail: the accurate central reference (~3.83) no longer shares truncation error with the still- forward internal panel block (~34.70). That failure is the correct unmasking of the ~9x error, but it can only be made green by Step 3 (correct the internal block), which is gated on the Step-2 measurement that was deferred. Removing the pin now would knowingly commit a red test, so instead the stale/false comment was corrected and the pin relabelled as an explicit temporary bridge tied to this issue.Next (unchanged from the recommendation): bump the fhsim package here → profile forward vs central panel block in a real net simulation (Step 2) → remove the pin and adopt the cheapest converging block (Step 3) only if measurement shows Jacobian quality moves wall-clock time.
1.9.8