feat: make Simulation.load() work, mostly - #350
Merged
Merged
Conversation
PR modflowpy#284 ("input file loading") prototyped recursive namefile loading; develop has since shipped that mechanism for real (Simulation.load()/ Gwf.load(), modflowpy#348/modflowpy#349), superseding modflowpy#284's implementation. This starts fresh off develop rather than rebasing, salvages the two still-useful pieces (a docs reconciliation, a new corpus smoke test), and uses that test to find and fix everything wrong with the shipped loader. test/mf6/test_mf6_load_all_models.py parametrizes Simulation.load() over every model modflow_devtools.models exposes. It found the loader succeeding on ~9% of the real corpus -- and several "successes" had silently wrong griddata values, not just crashes. Fixed, in order: - basic.lark: blank/comment-only lines inside a block, missing trailing newline at EOF, empty files, comma as a token separator (surfacing and fixing an unrelated pre-existing regex bug: a misplaced "-" was silently widening word's character class into an unintended range) - structure.py: GRIDDATA blocks were parsed through the wrong code path first, silently corrupting every DIS/DISV field (a bare `TOP\n CONSTANT 0.` collapsed to the boolean True, broadcast as 1.0); INTERNAL arrays wrapped across multiple physical lines (or carrying per-line trailing annotations) were truncated to one line; OPEN/CLOSE-redirected griddata and list/period row data were unsupported; a variable-width cellid's element count was guessed from row length instead of read unambiguously from grid dims; TIMEARRAYSERIES/AUXILIARY-named period fields weren't recognized - reader: non-MF6 legacy content trailing the last real block (an mf5to15 conversion leftover) is now trimmed as a parse-failure fallback, never on the normal path; a block name repeating in one file now keeps the first occurrence instead of silently losing it to the second Every fix verified against real parsed values, not just non-crash, and checked for regressions via a full corpus probe plus the full test suite before moving to the next. Corpus pass rate: 9% -> 99% (239/242). The remaining 3 are confirmed fixture-quality issues, not flopy4 gaps (root-cause trail in load-corpus-gaps.md, untracked, same convention as plan.md): an LGR test case's external array file sized for the wrong sub-model's grid, and a third-party-exported .oc file omitting a field every one of the corpus's other 108 uses of it supplies correctly. Full suite: 643 passed throughout, same 7 pre-existing unrelated failures, zero regressions introduced at any step. Where this leaves the typed-grammar work (dfn2lark/TypedTransformer, mf6-object-model-plan.md Phase 3): untouched, still not on the load path -- develop's Simulation.load() remains basic-grammar plus this now much more tolerant structure.py reconstruction layer. Worth noting for whenever Phase 3 is picked up: most of what this branch fixed was real MF6 files being messier than a strict spec-driven grammar would accept (trailing inline annotations, multi-line-wrapped arrays, no final newline, duplicate blocks, comma-separated values, fields predating a DFN revision) -- a generated typed grammar would need equivalent leniency to preserve this pass rate, not just better structure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QM8E6Gyw13cFKJDt6WFaHx
wpbonelli
changed the title
wpbonelli
added
documentation
load-corpus-gaps.md modflowpy#13. _read_open_close_values raised explicitly for "OPEN/CLOSE ... (BINARY)" since the gap-modflowpy#2 pass -- not a corpus failure (0/242 real modflow-devtools models use it, checked directly) but a real MF6 input capability with no coverage. Implemented per MF6's binary-array record format: a 52-byte header (KSTP, KPER int4; PERTIM, TOTIM float8; TEXT 16-byte label; NCOL, NROW, ILAY int4) followed by NROW*NCOL values -- real arrays double-precision, integer 4-byte. This is the exact layout flopy4's own binary head-output reader already decodes (utils/heads_reader.py::read_hds_timestep, "f.seek(52, 1) # skip kstp, kper, pertime") -- MF6 has used one binary 2D-array record format for both real and integer arrays since MODFLOW-2005 (u2drel/u2dint), for output and OPEN/CLOSE (BINARY) input alike. No real fixture exists to verify against, so: wrote a synthetic file in this exact format and round-tripped it through _read_open_close_values directly (real array, integer array with FACTOR scaling), then end-to-end through Dis.load() with a real .dis file referencing a synthetic binary TOP array, confirming byte-exact match. Added 3 permanent regression tests (test/test_package_load.py): a plain real array, an integer array with FACTOR, and a LAYERED field with one binary file per layer (matching the existing per-layer text-CONSTANT/INTERNAL convention already in place for LAYERED fields). Corpus-wide: unchanged, 239/242 (expected -- nothing in the corpus exercises this path either way), zero regressions. Full suite: 646 passed (643 + 3 new), same 7 pre-existing unrelated failures. Documented in load-corpus-gaps.md as gap modflowpy#13, flagged clearly: this is verified against a synthetic file built to the documented/inferred format, not against a binary file MF6 itself ever wrote or read. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01976PGtWgHyktqgMZY7PxPz
…ainst real MF6 fixtures load-corpus-gaps.md modflowpy#13 update. Verified the prior commit's binary-array support against real MF6 test fixtures, not just a synthetic file: ../modflow6/autotest/test_gwf_utl01_binaryinput.py is MF6's own test for binary array input, covering both real shapes (one binary file per layer, and one flat file spanning all layers). Ran its build_models() directly with real flopy 3.10.0 (flopy.utils.BinaryHeader.create (bintype="HEAD", ...) / Util2d.write_bin -- the actual writer real MF6 reads input from) to generate real fixtures, then loaded both through Simulation.load(). Confirms the header-format assumption exactly (bintype="HEAD" is the same 52-byte record layout already used); every field (top, botm, idomain, icelltype, strt) matched what build_models() wrote, for both fixtures. This caught a real bug the synthetic-only testing had missed: flopy3's writer quotes OPEN/CLOSE filenames (OPEN/CLOSE 'top.bin' ...), and word's tokenizer keeps the quote characters as part of the token (' is a valid filename character in the class), so workspace / "'top.bin'" (quotes included) never resolved -- FileNotFoundError. Fixed with a new _strip_quotes(), applied to both griddata (_read_open_close_values) and list/period (_resolve_open_close_rows) filename resolution -- affects the plain-text OPEN/CLOSE path too, not just BINARY. 0/242 corpus fixtures hit this on a *recognized* field: 35 corpus lines do quote an OPEN/CLOSE filename, but all 35 are for a DISU AREA field flopy4 has no hand-written class for yet, so the row is skipped before the filename is ever resolved (confirmed directly, not assumed). Corpus-wide: unchanged, 239/242, zero regressions -- consistent with that finding. Added a 4th regression test (test/test_package_load.py) for a single-quoted filename on both the binary and plain-text path. Full suite: 647 passed (+4 total across this and the prior binary-support commit), same 7 pre-existing unrelated failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01976PGtWgHyktqgMZY7PxPz
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.
#284 prototyped input file loading. The develop branch has superseded some of it. This PR salvages some docs and tests loading the examples and test models, and fixes some issues with the basic (untyped) loader.
Future work will benchmark the basic loader and prototype/benchmark the typed loader to decide whether it's called for