Uh oh!
There was an error while loading. Please reload this page.
Restore genForeFireCase.py as a general landscape writer - #179
Open
HugoFara wants to merge 1 commit into
Open
Conversation
The script that writes the NetCDF landscape file was deleted in ac0baba while four documents kept describing it, so anyone following the docs to build their first case hit a file that was not there. Restored from ac0baba^ with three fixes, since it was not usable as it stood: - The 3-D and 4-D paths were broken. Dimensions were read off the array as (NY, NX, NZ, NT) while the variable was created as (NT, NZ, NY, NX) and assigned untransposed, so a 4-D field raised a broadcast error and a 3-D one never had its NT dimension created. Fields are now indexed outermost axis first, which is what prealCF2Case.py in the same directory settled on when it fixed its own copy. - scipy.io.netcdf is deprecated and scipy is not a project dependency. Uses netCDF4, which the tests already require, writing the same NETCDF3_CLASSIC format prealCF2Case.py writes. - parametersProperties has seven required keys and the docs called them optional. They are checked before the file is opened, and named in the error, rather than raising a bare KeyError over a half-written file. tests/python/test_genforefirecase.py builds a landscape, loads it in ForeFire, ignites and steps, and pins the 3-D and 4-D shapes. The four documents now describe what exists. prealCF2Case.py still carries its own copy of both functions. Merging them is a change to a working script and is left for its own commit. Closes#167
HugoFaraforce-pushed
the
fix/restore-genforefirecase
branch
from
August 12, 2026 19:17
be2af01 to
41af42eCompareHugoFara
marked this pull request as draft
August 12, 2026 19:19
HugoFara
marked this pull request as ready for review
August 12, 2026 19:19
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.
Addresses #167. @filippi is the reviewer because he removed this script, and this proposes putting it back — that call is his.
The removal was intentional
I filed #167 saying the script was documented in four places and present in none, which is true, but I framed it as an oversight. The history says otherwise:
Two months apart, consumer first. The function had been inlined where it was actually used, and the standalone file was then removed as dead code. That is ordinary de-duplication, and this pull request undoes it.
Why I think it is still worth doing
prealCF2Case.py's copy has since diverged into something specialised:domainandparametersattributes are set by iterating the dicts — no required-key check, no float coercionf4rather thanf8windvariable in a potential-field form (NAXES, NDIRCOEFFS, NI, NJ), with thewindU/windVcalls commented out above itIt is now the back half of the Meso-NH PGD → case pipeline, and it needs a PGD file to do anything. So there is no general-purpose landscape writer in the repository, while
docs/source/user_guide/landscape_file.rst,tools/README.md,tools/preprocessing/READMEscripts.mdand the legacy user guide all describe one. Users following the documentation to build their first case have nothing to reach for.The cheaper alternative is to delete the four references. That respects
ac0babamost directly and takes minutes. It also leaves a new user with no path to a landscape file, which is why I am proposing this instead — but if the answer is "we removed it on purpose, fix the docs", that is a good answer, say so and I will close this and send that patch instead. No need to justify the original decision.What is in here, if it does land
Restored from
ac0baba^with three fixes, since it was not usable as it stood.The 3-D and 4-D paths were broken. Dimensions were read off the array as
(NY, NX, NZ, NT)while the variable was created as(NT, NZ, NY, NX)and assigned without transposing. Only the 2-D path was right. Running the original on fields with four distinct lengths:The 3-D branch created
NZbut neverNT, so it failed at variable creation rather than at assignment.Fields are now indexed outermost axis first —
(NY, NX),(NZ, NY, NX),(NT, NZ, NY, NX)— which is the conventionprealCF2Case.pysettled on when it fixed its own copy, with the old lines left commented out above the working ones. Two scripts in one directory with opposite conventions would be worse than either alone.scipy.io.netcdfis deprecated, and scipy is not a dependency of this project. UsesnetCDF4, which the tests already require, writing the sameNETCDF3_CLASSICformatprealCF2Case.pywrites.parametersPropertieshas seven required keys —date,duration,refYear,refDay,year,month,day— whichtools/README.mdcalled "the other optional properties you may want to put in the list". Omitting one raised a bareKeyErrorover a half-written file. They are checked before the file is opened, and the error names what is missing.Verification
tests/python/test_genforefirecase.py(new): builds a landscape, hands it to ForeFire, ignites and steps; checks the 3-D and 4-D shapes and values; checks that a missing parameter key is reported before anything is written. Four cases, all passing. The shape cases use four different lengths, sinceNY == NTandNX == NZis what let the old broadcast succeed by accident.It skips the load-and-simulate case when
pyforefireis not importable, so it stays runnable while iterating on the writer alone.Documentation
The three live documents now describe what exists.
docs/_legacy/UserGuide/ForeFireGeneralUsage.texalso mentions the script, at a stale path, and is left alone deliberately — editing an archived document would suggest it is maintained.Unifying the two implementations is not attempted here. They now do different things, and choosing between the combined
windvariable and separatewindU/windVis a question about what ForeFire should read, not a cleanup.This pull request, including its code changes and this description, was generated by Claude Opus 5, and reviewed manually before submitting.