Convection discretization rewrite#23
Open
speth wants to merge 37 commits into
Open
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add test/convergence/run_baselines.py and compare_baselines.py, plus captured baseline JSON for the six curated example cases (single, diffusion, twin, cylindrical outward/inward, laminarFlameSpeed) from commit 5552b7d, the pre-implementation tip of this branch. These will be diffed against the modified convection solver in later phases.
Add the shared advective-derivative kernel for the convection operator, with a bit-identical FirstOrderUpwind path and a van Albada limited, upwind-biased SecondOrderLimited path (spec 3.1-3.2). Includes gtest unit tests covering constant/linear exactness, bit-identical legacy parity, ~2nd-order convergence on smoothly nonuniform grids, no-new-extrema on monotone data, and upwind-branch selection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013DZnCpPG1q1crycu8GPukv
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
Add General.convectionScheme StringOption (default "secondOrderLimited", allowed value "firstOrderUpwind") and plumb it through to ConfigOptions::convectionScheme, mirroring the splittingMethod pattern. Parsing the string to the ConvectionDifferencer::Scheme enum is Task 1.3. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
…ontinuity Replace the inline dTdx/dUdx/dWdx upwind loop in ConvectionSystemUTW::f() with three ConvectionDifferencer::computeDerivatives() calls (branch selection on rV, unchanged sign convention), and add the spec 3.3 trapezoidal continuity march gated on SecondOrderLimited in all continuity-BC branches (Left forward; Zero forward+backward; Temp/Right/ Qdot forward+backward). FirstOrderUpwind keeps the legacy rectangle rule verbatim. The differencer is a per-instance member of ConvectionSystemUTW, resized alongside the system. ConvectionSystemSplit::setTolerances() parses convectionScheme to the enum (throwing DebugException on unknown values), stores it, and propagates it to utwSystem. Verification: firstOrderUpwind is bit-identical to pre-change behavior. Spot-check ran the test_flame_configs TestPremixedStrained config (nThreads=1, convectionScheme='firstOrderUpwind') on the working tree vs a clean build of pre-change commit f2eafc1; final T and x profiles matched exactly (max|dT|=0.0, arrays bit-identical). gtest 27/27 and pytest 15/15 pass (the flame-config integration tests run under the default secondOrderLimited scheme). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
…-2D velocity fix Route ConvectionSystemY::f() through the per-instance ConvectionDifferencer kernel (one call per f() evaluation) instead of the inline upwind loop. Each species system owns its differencer and scratch vectors for TBB safety. The standard path selects the upwind branch from the interpolated normal velocity v as before; the quasi-2D path builds a node-wise velocity array from vrInterp and passes it to the kernel, fixing the legacy uninitialized-v upwind-direction bug (spec 3.4). Boundary rows (j=0 inflow balance, j=jj outflow) and the quasi-2D ydot assembly form are unchanged. The scheme is propagated to each species system in ConvectionSystemSplit::configureSolver(). FirstOrderUpwind remains bit-identical (verified: full final state parity against 30a4c3c, max abs diff 0.0 for x, T, and all species Y). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
Confirms firstOrderUpwind parity against the Phase 0 baselines (all six curated cases pass compare_baselines.py's 0.02 threshold, five of six at or below the ~1e-6 reproducibility floor) and characterizes secondOrderLimited's deltas (small, physically consistent shifts; no sign changes, negative Y, NaNs, or CVODE step-count blowup). Extends run_baselines.py with a --scheme override flag and a total_convection_steps field (summed from the per-timestep [C: N] debug log lines) to support both checks. Also documents an example_single-specific finding: same-code repeats show termination-time sensitivity (thread-scheduling noise pushes the steady-heat-release termination check to noticeably different sim times), producing run-to-run profile differences larger than the previously-recorded floor for that one case. Not a scheme-induced regression, but flagged for follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
Close the final whole-branch review findings: smoke-test the previously never-executed stagnationPoint/fixedTemperature continuity branches under both convection schemes (4/4 pass, results in test/convergence/README.md), fix the stale species-propagation comment, document the Temp-BC anchor's first-order inversion under secondOrderLimited (accepted per spec 3.2), and reconcile the ConvectionDifferencer::resize() doc with the defensive self-sizing fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
Add run_convergence.py (case/scheme/rung-selectable harness for the spec §6.4 grid-convergence study: strained/twin/cylindrical cases, 6-rung vtol/dvtol/gridMax ladder, --damp-const trial support) and plot_convergence.py (error-vs-N plots referenced to the finest secondOrderLimited run). Smoke-tested the coarsest rung of all three cases; full study matrix is Task 2.2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
Full (case x scheme x rung) matrix for the grid-convergence study (spec §6.4): strained/twin/cylindrical x firstOrderUpwind/secondOrderLimited x 6-rung ladder, plus a dampConst=15 relaxation trial on strained/secondOrderLimited. 33/36 main-matrix runs and 5/6 dampConst-trial runs completed; strained/secondOrderLimited rung5 and cylindrical/secondOrderLimited rungs 4-5 fail deterministically (CVODE Integrator had too many errors, reproducible under nThreads=1) while firstOrderUpwind completes cleanly at the same tolerances for both cases -- flagged for the analysis stage, not worked around. No RUNGS retune was needed/performed (twin and firstOrderUpwind both hit the full ~4-8x N span per the existing ladder). All 6 case/metric plots generated via plot_convergence.py and verified non-empty. Full run table, dampConst comparison, and sanity-gate self-review in .superpowers/sdd/task-2.2-runs-report.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
Append "Phase 2 findings" (§P2.1-P2.7) to the convection-discretization design spec: observed convergence orders, error-vs-N grid-reduction tables, CVODE step-count/limiter-smoothness comparison (spec §5), diagnosis of the deterministic CVODE failures at the finest rungs, the dampConst null result, and the Phase 3 gate assessment. Headline: secondOrderLimited buys a large, consistent per-grid-point accuracy gain (peak_T grid-converged at the coarsest grid in all cases; ~7x grid reduction at matched consumption_speed for strained/twin). The spec §5 step-count red flag has triggered: a deterministic grid-refinement/limiter- stiffness feedback runs CVODE away to failure at the finest rungs of the two high-strain cases. Analysis-only diagnostic probes (grid-frozen and intermediate-rung, no src edits) locate the mechanism and a stable fixed point just below the failing rung; JSONs under test/convergence/results/diagnostics/. Recommendation (formulated, not decided): Phase 3 gate OPEN on the diffusion- dominance criterion, conditioned on first fixing the tight-tolerance CVODE robustness bug. Decision: PENDING user review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
…ostic Record the project owner's decision in the spec addendum: Phase 3 gate OPEN (useful accuracy envelope ~1e-4 on scalars, reached at N~100; diffusion upgrade judged worthwhile), with the tight-tolerance robustness fix routed through a short design pass. Post-review diagnostic rules out the QSS chemistry integrator as the CVODE-failure source: strained/sol/rung5 fails identically with chemistryIntegrator='cvode' (grid 496->537 non-settling, convection CVODE ~2600-2900 steps/global step, err_convectionIntegration.h5). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
…as primary candidate Owner direction for the robustness/Phase-3 design pass: evaluate monitor-function/error-equidistribution grid adaptation (dimensional error density replacing vtol/dvtol relative-change) as the primary candidate, against the hysteresis+growth-cap patch. Adjoint/QoI-driven refinement is deliberately out of scope for Ember and earmarked for Cantera. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CxCXLfj7MZ3qNs5eHXeXZ
Replaces vtol/dvtol relative-change criteria with a scheme-aware local truncation error budget (range-normalized errTol), per owner decisions: replace-with-deprecation, hard acceptance on the §P2.4 CVODE failures, limiter smoothing in scope as contingency. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…x scope, envelope regression criterion Pre-plan corrections: the approved h^p error form was dimensionally inconsistent (v/length vs. the errTol*range(v) threshold); corrected to the standard degree-p interpolation-error bound h^(p+1)*|d^(p+1)v| with C_1=1/8, C_2=1/15 starting constants and a documented sub-linear tol->QoI-error mapping. Scoped in a minimal fix for the pre-existing dampVal uninitialized-read in adapt(). Regression criterion restated in accuracy-envelope terms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…l; dampVal kept consistent in adapt() Replace the relative-change vtol/dvtol insertion/removal criteria in OneDimGrid::adapt() with the local-error-budget criterion from G1 (computeErrorWeights/errTol/errorOrder/errCoeff): insert where the estimated local representation error exceeds errTol*range(v); remove only where the merged-interval error estimate stays below rmTol*errTol*range(v), giving removal built-in hysteresis since error scales as h^(p+1). Also fixes a real bug: updateValues() resizes the dampVal Eigen array without guaranteeing content preservation once points have been inserted/removed mid-pass, so the damping criterion could read stale values. adapt() now maintains a std::vector working copy (dampLocal) kept in sync via addPoint/removePoint-mirroring insert/erase calls, and writes it back to dampVal at the end of the pass. Removes ConfigOptions::vtol/dvtol and OneDimGrid's vtol_in/dvtol_in/ vtol/dvtol members; adds ConfigOptions::errTol and Grid.errTol (FloatOption(2e-3, min=0)) in input.py, forwarded via _ember.pxd/.pyx. vtol/dvtol remain as (now-unforwarded) input.py Options pending G3's deprecation handling. test/python/test_flame_configs.py: replaced Grid(vtol=0.2, dvtol=0.3) (3 occurrences) with Grid(errTol=3.3e-3), using the same vtol:errTol ratio as the provisional default mapping, since vtol/dvtol are no longer forwarded to the C++ grid and the old call would have silently fallen back to the default errTol. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpMZDeJn1DXH5NshhtCDqq
…ple tail) + dense profile output Review fixes: n_trajectory now globs prof[0-9]*.h5 so the continuously rewritten profNow.h5 no longer inflates counts or injects an off-cadence final sample; grid_settled floor lowered to 4 samples with a >=3-sample tail window and a distinct TOO FEW OUTPUTS report; strained/twin builders now set profileStepInterval=10 (matching cylindrical) so trajectories are dense enough to judge. Validated: strained rung-2 smoke runs (both schemes) complete, 23 outputs, SETTLED, sol N=87 < fou N=122; P2.4 failure dir reports NOT SETTLED; old passing rung-3 dir reports SETTLED. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cases Parity calibration of the secondOrderLimited error coefficient against firstOrderUpwind at matched errTol (rungs 1-4, 3 cases, both schemes): R = err_fou/err_sol = 2.85 (geometric mean, consumption speed), outside the [0.5, 2] keep-band, so C_2 = 1/15 -> 0.0139 = (1/15)*R^(-3/2). Note: the brief's rule stated err_sol ~ C_2^(2/3); the sign is inverted (larger errCoeff -> finer grid -> smaller error), so the R^(-3/2) scaling implements the brief's stated intent (multiply err_sol by R). Confirmed empirically on strained rungs 2-4 (grids coarsen, mean error move x2.5 vs predicted x2.85; fou control bit-identical). Default Grid.errTol = 1e-4 (was 2e-3), chosen from measured (errTol, N, err_cs) triplets with default gridMax: N = 115-165, err_cs = 3.1-6.5e-4 on the study cases. The owner envelope (err ~ 1e-4 at N ~ 100) is not jointly attainable; 1e-4 is the balanced one-sig-fig point and sits well inside the observed stability region. Docstring examples updated to 2e-5 (high accuracy, verified stable on strained with default gridMax) / 5e-4 (minimal). Details in test/convergence/results/calibration-notes.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpMZDeJn1DXH5NshhtCDqq
…l->error tables Ran the full errTol ladder (rungs 0-5, 3 cases x 2 schemes, 36 runs) at G5's post-calibration constants. Hard criterion PASSES: zero CVODE failures anywhere, including the two configs that failed deterministically under old vtol (strained sol rung 5, cylindrical sol rungs 4-5); grid trajectories all overshoot-then-plateau with bounded, decaying convection step counts (vs. the old divergence into 10,000-16,000). Parity geo-mean R=1.70 (within 2x band, improved from pre-cal 2.85), with an anomaly flagged for strained's persistently >2x per-rung ratio. plot_convergence.py gains the errTol parity view, the errTol->error table, and the sol/fou N-ratio table (the vtol->errTol metadata migration itself was already a no-op per G4). Full verdict, tables, and anomalies in spec addendum §A. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpMZDeJn1DXH5NshhtCDqq
…conciliation Review fixes to spec addendum §A: (1) parity verdict relabeled PARTIAL -- within ~2x in aggregate (R=1.70) and for twin/cylindrical individually, but strained exceeds the band at every rung 1-4 (2.67-5.40x), which the spec's drift-at-extremes language does not license; documents that no single global C_2 can bring all three cases inside 2x simultaneously (pre-cal per-case geomeans spanned ~5x) and lists the three options as an owner decision. Hard-criterion PASS unchanged. (2) §A.4 envelope-range reconciliation sentence: the wider N/err span here covers all cases/ schemes/QoIs, vs. calibration-notes.md §4's sol-only consumption-speed span at errTol=1e-4 -- same operating point, different scope. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpMZDeJn1DXH5NshhtCDqq
Reran the Phase-1 curated baseline set at library defaults (errTol=1e-4, secondOrderLimited) and compared against the Phase-1 secondOrderLimited baselines. 5/6 cases pass the amended spec Sec4 scalar criterion with wide margin (peak_T/consumption_speed deltas <=1.24e-3), but N grew 10-23% for every completing case, and example_single fails to complete outright (deterministic CVODE Integrator error, reproduced at nThreads=1, cleared by loosening errTol to 2e-4/5e-4) -- a robustness gap outside the G1-G6 three-case ladder, surfaced here for owner review alongside the other pending errTol=1e-4 default questions. Not tuned away per instruction. Also: retune the three test_flame_configs.py Grid(errTol=...) call sites and example_laminarFlameSpeed.py's default config to the post-G5-calibration errCoeff (both were still tuned against the pre-calibration C_2, ~2.85x too tight); fix input.py's stale rmTol docstring (compares against errTol, not vtol/dvtol); and replace analyze_settling.py's fixed tol_pts=3 with a tail-size-relative tolerance, since the absolute floor was flagging plateaued high-N ladder runs as NOT SETTLED. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpMZDeJn1DXH5NshhtCDqq
Ten pre-presentation fixes from the final whole-branch review: 1. run_baselines.py: drop dead vtol/dvtol kwargs from the laminarFlameSpeed case (G7 baseline actually ran at default errTol=1e-4); note that intent. 2. smoke_continuity_bc.py: replace inert Grid(vtol=0.2, dvtol=0.3) with Grid(errTol=1.2e-3) to match the TestPremixedStrained tolerance it derives from. 3. Correct the dampVal narrative: final review confirmed (via git show 5d4febe:src/grid.cpp) that base addPoint/removePoint already maintained dampVal, so updateValues()'s resize was always a same-size no-op and no uninitialized read existed. Rewrote the grid.cpp comment and the design doc to retract the "pre-existing bug" diagnosis and describe dampLocal as defensive hardening; appended a local progress-ledger note. 4. input.py: qualify the errTol docstring and deprecation-warning text — accuracy parity is PARTIAL (strained cases run 2.7-5.4x above the 2x band), so both now note that highly strained flames may need a tighter errTol under firstOrderUpwind. 5. input.py: errTol option now enforces min=1e-12 instead of min=0, so it can no longer degenerate to refine-everywhere. 6. input.py: run('force') and runESR bypass validate(); route _warnDeprecated() through evaluate() (the common config->solver-options path) with a _deprecationWarned guard so it fires exactly once. 7. test_gridAdaptation.cpp: new test pinning setOptions()'s convectionScheme -> (errorOrder, errCoeff) mapping (1/0.125, 2/0.0139). 8. grid.cpp adapt(): assert dampVal.rows() == nPoints alongside the existing positivity assert, making the caller contract explicit. 9. grid.cpp: removal-loop damping debug log now prints the actual threshold used in the comparison (rmTol*dampConst*dampLocal[j]). 10. example_laminarFlameSpeed.py: comment noting errTol=5e-4 is the docstring's "minimal accuracy" tier, chosen to preserve historical resolution/runtime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpMZDeJn1DXH5NshhtCDqq
…ly initialize ConfigOptions in mapping test Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
No description provided.