Uh oh!
There was an error while loading. Please reload this page.
Recover the double-delete and moisture fixes that missed dev - #173
Merged
Conversation
`properties` is allocated by each model's own constructor with `new double[numProperties]` and freed by ~ForeFireModel. Sixteen flux models and two propagation models freed it again in their own destructor, so destroying any model that registers at least one property was a double free. The flux ones also used scalar `delete` on an array allocated with `new[]`. Removing the derived deletes and leaving it to the base class is the whole fix; no ownership changes. This has never been hit in production because FireDomain keeps its models in propModelsTable and fluxModelsTable and frees neither, so nothing destroys a model today. It is reachable from anything that does, including a test -- which is why the test that covers it comes with it: `every model can be destroyed` now runs all 33 models rather than the six that register no properties, and `destroying a model does not disturb the next one` allocates across a destruction, since a double free often surfaces as the next allocation coming back wrong rather than as an immediate abort. Verified by reintroducing a single delete: the suite aborts with "double free or corruption (!prev)" and doctest reports the case as CRASHED - SIGABRT.
Two rate-of-spread models misbehave once dead fuel moisture reaches or exceeds the moisture of extinction. Neither is reachable today, because Md is a static column of the fuel table that never gets that high, but both become reachable the moment dead fuel moisture is driven by a dynamic field. RothermelAndrews2018 returned NaN. Its moisture damping cubic is only defined below the moisture of extinction: it reaches zero at mf/me == 1 and turns negative past it, driving the reaction intensity negative, then the wind limit `wv = 0.9 * RI` negative, then pow(wv, B) to NaN for the non-integer B. The `R <= 0` exit test is false for NaN, so the value escaped into FireNode::velocity. Return zero before the cubic is evaluated, and negate the exit test so a NaN can no longer slip past it. The Balbi family sped up instead of slowing down. xsi is the share of combustion energy spent vaporising the moisture the live fuel carries in excess of the dead fuel; when the dead fuel is the wetter of the two it goes negative, so (1-xsi) exceeds 1 and amplifies both the radiant term and the flame temperature, which R00 raises to the fourth power. At Md=3.0 that put the flame temperature near 18000 K and made the fire spread 33x faster than at Md=1.0. Floor xsi at zero: the dead fuel's own penalty is already carried by the 1/(1 + a*Md) factor. Applied to all five models that share the expression. xsi is deliberately not capped above. With the current fuel tables it reaches ~5.3, which makes the flame temperature negative, but that traces back to Deltah equalling DeltaH in most rows of fuels.csv rather than to the model, and capping would move every existing calibration. Behaviour below the thresholds is unchanged to the last digit, and the runff KML and NetCDF references still match.
The runff test compares ForeFire against frozen ForeFire output, so it cannot tell a physics fix from a physics regression: any deliberate change to a spread model breaks it, and any accidental one that leaves real_case.ff untouched slips through. This suite holds no reference data. Every assertion follows analytically from the published spread equations, so it stays valid across recalibration and across the planned switch from a static fuel.Md column to a dynamic dead-moisture field. Five invariants, over Rothermel, RothermelAndrews2018 and BalbiNov2011: finite ROS is a real number for every moisture in [0, 3] monotonic ROS decreases as dead fuel moisture rises extinction ROS reaches zero at Md >= me and stays there responsive a dynamic dead-moisture layer reaches the model resolved DataBroker finds an optimised getter for every property `responsive` skips until dynamic dead moisture exists, then activates. `resolved` exists because DataBroker does not raise on an unknown property: it warns and silently falls back to an un-optimised path, so a mistyped property name yields stale values rather than an error. Two constraints the probe has to respect. Each probe runs in its own interpreter, because the core keeps mutable global state and a second ForeFire() in one process inherits the first one's parameters -- an in-process sweep returns one identical displacement for every input. And relax is forced to 1, because below that FireNode::update blends in the ignition velocity, which decays geometrically but never reaches zero, so an extinguished front still creeps and extinction is unobservable. Each assertion was mutation-checked: flipping the sign of the moisture damping term, applying the dead-moisture layer inverted, removing the moisture-of-extinction cutoff and mistyping a property name each turn the matching invariant red. Runs in about 16 seconds.
registerPropagationModel matched the prefix "moist" and routed anything starting with it to getMoisturesProperties, which fills five slots from the moistures.* scalar parameters. That getter exists for the NFDRS-style group FarsitePropagationModel registers as moist.ones / moist.tens / moist.hundreds / moist.liveh / moist.livew, where five reserved slots match five written values. The prefix also caught the singular "moisture", which reserves one slot. Balbi2015, Balbi2020 and BalbiNov2011TMdMl each register it, so every property they registered afterwards was written four slots too late and the last four ran past the end of the properties array. Balbi2020 read its temperature out of moistures.liveh, its Rhod out of moistures.tens and its Md out of moistures.hundreds, and wrote 28 doubles into a 24-element allocation. Measured, it spread the same 42.43 m -- the distance covered by the initial front alone -- at every fuel-table Md from 0.02 to 0.40: the model could not see its own fuel table. Match "moist." instead, so the group still reaches the five-slot getter while "moisture" resolves through propPropertiesGetters to getMoisture, which writes exactly one value. That leaves getMoisture reachable for those three models for the first time, and it dereferenced moistureLayer unconditionally. Guard both overloads and fall back to the moistures.ones parameter, which is the value these models were already receiving, so no configured simulation changes its numbers; warn once, since a model asking for a moisture field and getting a constant is almost certainly misconfigured. The runff KML and NetCDF references are unaffected: Rothermel registers no moisture property.
Balbi2020, Balbi2015 and BalbiNov2011TMdMl each register a "moisture" property beside their fuel properties, which is what the property-shift fix addresses. Supply a temperature and a moisture layer from the probe so all three can run, and add them to the sweep. Reverting the "moist." prefix narrowing turns Balbi2020's monotonicity red, so the fix is now covered. Run duration is calibrated per model rather than fixed. Spread rates differ by more than two orders of magnitude across these six models, and one duration either drives the fast ones into the domain edge, clipping the dry end of the sweep, or leaves the slow ones so close to the initial front that perimeterResolution quantises the displacement and adjacent moistures collide. The calibration iterates, because a Balbi front accelerates as it develops and a rate measured over a short trial underestimates the eventual one. All six models now land between 350 and 715 m in a 2000 m domain, well clear of both failure modes, and the sweep keeps resolving if any model is recalibrated later. Runs in about 40 seconds.
Adds invariants.yml, which builds the pyforefire extension and runs tests/python/test_moisture_invariants.py on pushes to master, pull requests against it, and manual dispatch. A separate workflow rather than a job in main.yml, because the suite needs the Python extension and install-forefire.sh does not build it: outside a wheel build CMakeLists defaults FOREFIRE_BUILD_PYTHON to OFF, so main.yml produces only the native interpreter. The job installs the package into a venv instead of system Python, which keeps it clear of PEP 668, and pip's build isolation supplies scikit-build-core and pybind11 from pyproject.toml. Checkout skips LFS, unlike main.yml: the suite builds its fuel, wind, temperature and moisture layers in memory and needs no fixtures. Build and suite together take about a minute locally. This gives the repository a check that can distinguish a physics fix from a physics regression. The runff test compares ForeFire against frozen ForeFire output, so a deliberate model change breaks it while an accidental one that leaves real_case.ff untouched passes. Also documents the suite in TESTING.md, including the requirement that each probe run in its own interpreter, and corrects the claim there that nothing under tests/python is validated in CI.
Matches the change made to main.yml and macos.yml: the workflow only triggered on master, so nothing integrating into dev would run it.
Keep rate of spread finite and damped at high fuel moisture
…te' into recover/stacked-fixes
Uh oh!
There was an error while loading. Please reload this page.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
#157 and #158 are marked merged, but their changes are not on
dev. The stack was merged bottom-first, so the upper PRs landed on branches that had already been consumed.dev?devtest/model-unit-suitefix/model-properties-double-deleteSo
devcurrently has the unit test suite, but not the double-delete fix and not the moisture guards.Verified on
origin/devat8b8f5b1:What this PR does
Merges
fix/model-properties-double-delete(which carries both #157 and #158) into currentdev.This is a merge, not a fast-forward, deliberately:
devhas four commits the stack never saw — includingMerge branch 'master' into devand thedocs/_legacyreorganisation. Fast-forwarding would have revertedtools/devops/install-netcdf-manylinux.sh. The merge preserves both sides and applied with no conflicts.Verification
runff: KML and NetCDF both within tolerance.dev's newer work is untouched — no diff againsttools/devops/ordocs/.For next time
When merging a stack, merge top-down — #158 into #157's branch, then #157 into #156's branch, then #156 into
dev— or retarget each PR todevas the one below it lands. GitHub retargets automatically on merge, but only if the lower PR merges first and the upper one has not already been merged elsewhere.Drafted by Claude Opus 5. Reviewed by a maintainer before filing.