Uh oh!
There was an error while loading. Please reload this page.
Make runANN work, and run it in CI - #183
Merged
Merged
Conversation
runANN has failed on line 2 since it was committed. It diffs the model predictions against result.txt.ref, and that file is not in the repository: diff: result.txt.ref: No such file or directory ANN results differs from reference. Renaming the committed result.txt would not have fixed it. Regenerating that output changes a handful of lines in the last digit (12412.1 against 12412.2), so an exact diff is red on any machine but the one that produced the file. ANN_test already computes the root mean squared error between what the network predicts and what the model produced, and run.bash was throwing that line away with `grep result`. It is now what the test checks: no reference file, no dependence on the last digit, and it fails when the network stops approximating the model. The tolerance is calibrated against this fixture: trained network 0.0235 predicting the mean of every output 0.0966 normalisation weights scaled by 1% 17817 0.05 sits between the first two, so a network that ignores its inputs fails rather than passing. Anything that genuinely breaks the arithmetic lands orders of magnitude away. The fixture is weak, and that is recorded rather than papered over: the expected outputs take four distinct values spanning 1.1 in 12412, which is why the margin between a working network and a constant one is so narrow. Better inputs would make this a much stronger check. Wired into main.yml. This is the only coverage for ANNPropagationModel and BMapLoggerForANNTraining: both read a .ffann network in their constructor and abort without one, so the unit suite cannot construct them. ANN_test is already built there, since FOREFIRE_BUILD_TOOLS defaults on outside wheel builds. Also: - result.txt is removed. It was the golden file for a comparison that no longer exists, and ANN_test regenerates that output on demand with its `print` argument. - clean.bash deleted results.txt, which is not the name of anything the suite writes. There is nothing to clean now. - TESTING.md said runff was the only test validated in CI, which has not been true since the unit suite and the moisture invariants landed. Verified in ubuntu:24.04: same RMSE as the host to seven digits, which is the portability this replaces the exact diff for. Closes#163
HugoFara
commented
Aug 12, 2026
CollaboratorAuthor
Test only, merging. |
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 12, 2026
Merged
antonio-leblanc pushed a commit
that referenced
this pull request
Aug 13, 2026
The three cherry-picked commits describe master. This adds what this branch has that master does not: - The unit suite. tests/README.md said five sets of tests; with tests/unit there are six, and it is a CTest suite rather than something run.bash reaches, so the ctest command line is given. - runANN works here (#183). The master text says the suite fails because it diffs against a result.txt.ref that is not in the repository; that is replaced by what it now does -- an RMSE tolerance of 0.05, sitting between the trained network at 0.0235 and a mean-predictor at 0.0966. - test_moisture_invariants.py and test_threading.py exist here. Both are documented, including that test_threading.py does not pass and is not meant to: it is the reproduction for #175 and needs a free-threaded CPython. None of the three are pytest modules, so no pytest command is given. - The four build options this branch added -- FOREFIRE_BUILD_TESTS, FOREFIRE_ENABLE_WARNINGS, FOREFIRE_WARNINGS_AS_ERRORS and FOREFIRE_SANITIZE -- join the table in installation.rst, and the wheel-defaults sentence counts tests among what wheels turn off.
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
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#163.
runANNhas failed on line 2 since it was committed, and it is the only coverage forANNPropagationModelandBMapLoggerForANNTraining— both read a.ffannnetwork in their constructor and abort without one, so the C++ unit suite cannot construct them.Why a rename would not have fixed it
result.txt.refis not in the repository, and it looks likeresult.txtwas committed in its place. But regenerating that output changes a handful of lines in the last digit:Five lines out of 1424 here. An exact
diffis red on any machine but the one that produced the file.What it checks now
ANN_testalready computes the root mean squared error between what the network predicts and what the propagation model produced — andrun.bashwas discarding that line withgrep result. That is now the check. No reference file, no dependence on the last digit.The tolerance is calibrated against this fixture rather than guessed:
The middle row is the one that set the number. My first attempt used 0.1, which a network ignoring its inputs entirely would have passed — a test that cannot tell those apart is not worth running. 0.05 leaves the real network a factor of two of headroom and still fails a constant predictor.
The fixture is weak, and that is recorded
The expected outputs take four distinct values spanning 1.1 in 12412. That is why the margin between a working network and a constant one is only a factor of four, and why perturbing a single normalisation weight by 5% moves the error by 2%.
The check is still meaningful — 1% damage to the normalisation layer takes the error to 17817 — but it is closer to a smoke test for the ANN path than to a numerical regression test. Replacing
modelrun.csvwith inputs that produce a real spread of rates of spread would fix that, and needs someone who knows what the fixture ought to represent.Also in here
result.txtis removed. It was the golden file for a comparison that no longer exists;ANN_test ... printregenerates that output on demand.clean.bashdeletedresults.txt, which is not the name of anything the suite writes. There is nothing to clean now.TESTING.mdsaidrunffwas the only test validated in CI. That stopped being true when the unit suite and the moisture invariants landed. It now documentsrunANN, the calibration table above, and whattests/run.bashactually runs.Not done here
tests/python/idealizedwind.py,farsite_flat.pyandpercolation.pyare listed in #163 as unwired. They are examples rather than tests — they produce plots and assert nothing — so wiring them into CI means deciding what they should assert first. They are documented inTESTING.mdas the worked examples they are, and left for their own change. Themnh_*cases need a Meso-NH build, which is its own question.Verification
runANNpasses on the host and inubuntu:24.04, with the same RMSE to seven digits — which is the portability this replaces the exact diff for.This pull request, including its code changes and this description, was generated by Claude Opus 5, and reviewed manually before submitting.