several more things - #349
Draft
wpbonelli wants to merge 9 commits into
Draft
Conversation
…trip
Bump modflow-devtools pin to a9d66f5 (MODFLOW-ORG/modflow-devtools
develop), which splits the old overloaded pk/fk field semantics into
index (needs MF6's 1-based/0-based conversion), node (grid-cell
reference), and pk/fk (pure relational identity). Follow the split
through flopy4: field() gains index=; codegen (list_columns(), row_class(),
_is_index()) reads dev3's real Integer.index directly instead of
inferring "needs the numeric shift" from isinstance(f, Integer) and
(pk or fk); row.py's to_row()/from_row() check metadata.get("index")
instead of pk-or-fk. Regenerated all 63 component modules against a
current modflow6 develop checkout, picking up real corpus drift (new
option fields, dfn_name registration for Api/Ist) along the way.
The regen surfaced one previously-silent bug: MVR's id1/id2 period
columns (DFN numeric_index: true, "the first well has an identifier of
one") never carried pk/fk upstream before this devtools change, so
flopy4 never applied the conversion for them -- callers had to pass an
already-1-based value, unlike every other index/fk column. test_gwf_mvr
updated to pass 0-based id1/id2 like everywhere else.
Also: preserve list/only-kind package pname (Chd, Wel, Dis, Ic, Npf, ...)
across load/write round trips. Simulation.models/exchanges/solutions
(dict-kind binding fields) already round-trip a custom pname via
xattree's own .name reconciliation, but list/only-kind fields don't --
xattree always reconciles those to a field-derived name regardless of
what's passed to the constructor, so a namefile's real pname silently
became "chd0"-style on the very next write. Add Component.pname, a plain
field xattree doesn't manage or reconcile (same category as the existing
filename field). _resolve_bindings sets it from a binding row's real
pname; Binding.from_component emits pname or name instead of always
name. .name itself is untouched -- xattree's own child-attachment
convention isn't part of this change.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zbgdnrF7fDHg8CudSgNHs
Row (row.py) and Record (record.py) were independent mixins with overlapping keyword/tagged-field serialization despite modeling the same underlying DFN concept: Row for a repeating tabular block's rows, Record for a standalone compound value. Item(Record) makes that relationship explicit -- Item adds only what's genuinely specific to being one member of a collection (index/pk/fk renumbering, cellid packing, aux/boundname, external ncelldim/naux/boundnames parse context), while reusing Record's keyword/coercion helpers for everything else. Found and fixed two real bugs while verifying against the full test suite: (1) Record's Union-unwrapping in _coerce was too eager and would force-convert Item's genuine multi-arm Union[float, str] fields (e.g. LAK's claktype) to float, breaking string values like "VERTICAL"; now only Optional[X]-shaped unions (a single non-None arm) are unwrapped. (2) An early attempt to unify tagged-field handling between Record and Item broke Item's flag-style tagged fields (e.g. LAK tables' MIXED), which are always Optional[str] presence markers rather than real bools -- reverted Item's tagged handling to its own dedicated logic, documented why the split is real rather than incidental. Generated files' `from flopy4.mf6.row import Row` become `from flopy4.mf6.item import Item as Row` as a mechanical, no-shim transition (no class bodies touched) -- full renaming to drop the "Row" suffix happens in the codegen stage of record-item-consolidation-plan.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SvRiB5PxAt8PQmo8m4bTw
…stance(Record) extra_list_blocks() and extra_period_fields() (overrides.py) had zero call sites -- dev3's Union.arms replaced what they were for (per the comment already at make.py:545), but the functions and their backing dfn_overrides.toml data (LAK/LKT/LKE period keystring entries) were never deleted. extra_list_blocks had no backing data at all. Also replaced two duck-typed "attrs.has(cls) and '_keyword' in vars(cls)" checks in unstructure.py with isinstance(x, Record), now that Record is an importable base class rather than a structural pattern to probe for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SvRiB5PxAt8PQmo8m4bTw
Codegen naming now matches Record's convention (no suffix repeating the base class's own name): generated list-block item classes are PackagedataRow -> Packagedata, ConnectiondataRow -> Connectiondata, etc., all subclassing Item directly. The period-block item class was previously hardcoded to the literal name "Row", needing a `_Row = Row` import alias just to dodge a mypy self-reference issue (`class Row(Row):` doesn't resolve). It's now named StressPeriodData, derived from the field's actual public name (stress_period_data) via a real snake_case -> PascalCase conversion (filters.pascal_name) rather than the previous bare str.capitalize() (which only worked because existing block names happened to be single words). Since no generated class is ever literally named "Item" anymore, the mypy-shadow alias is gone entirely. filters.row_class -> filters.item_class, registered as the `item_class` Jinja filter alongside the new `pascal_name` filter. Applied the same rename by hand to the handful of hand-written files the codegen pipeline doesn't own (tdis.py, and gwf/gwt/gwe/prt disv.py's Vertices item) for consistency. Regenerated all 63 components via `pixi run generate-classes` against the modflow6 develop DFNs; full test suite matches the pre-existing baseline exactly (523 passed, 21 pre-existing failures needing an mf6 binary this environment doesn't have, 12 skipped, 1 xfailed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SvRiB5PxAt8PQmo8m4bTw
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SvRiB5PxAt8PQmo8m4bTw
…l typed arms
Three pieces of work landed together because they all require the same
full codegen regeneration:
1. Record composition (item 3). A DFN record nested inside another
(the head/temperature/concentration/qoutflow/cim printrecord family:
outer record wraps formatrecord) now composes as its own class
(Concentrationprint.formatrecord: "Oc.Format") instead of flattening
the nested record's fields into the parent. can_generate_record_class
recurses arbitrarily deep instead of a hardcoded one-level limit.
Nested-ness is inferred from the field's own forward-reference type
annotation at runtime (Record._nested_class, cached) rather than a
declared "nested" metadata flag, since resolvability against a real
Record subclass is itself the signal.
2. Period keystring-union arms are now real typed Item classes (LAK's
Status/Stage/Rate/Auxiliary/..., OC's Save/Print, PRT-PRP's
All/First/Last/Frequency/Steps) instead of the generic
(index?, keyword, value) placeholder that collapsed every arm's real
shape into one untyped "value" column. Reuses the existing list-column
machinery (ColumnSpec/_schema_dict_from_columns/item_class) per arm,
handling all three index shapes in the corpus (OC: no index; LAK: index
embedded per-arm; SFR/MAW: index shared via an outer sibling field) and
disambiguating a per-arm class name collision by prefixing with the
parent field's name.
OC's per-rtype convenience constructor (save_head=, print_budget=, ...)
is gone; stress_period_data is now dict[int, list[Save | Print | ...]]
like every other keystring-union package. The old ""-stop-sentinel
mechanism needed no replacement -- the existing generic dict[kper,
list[Item]] fill-forward already does the right thing (each kper's
list is exactly what's written; omitting a setting from a later
period's list is how it "stops", same as LAK/SFR/MAW).
A nested union within an arm (OC's ocsetting, PRP's releasesetting --
ALL/FIRST/LAST/FREQUENCY n/STEPS n1 n2 ...) isn't recursively exploded
into further typed sub-arms; it's a new "array" field kind that
consumes all remaining tokens as a tuple (numeric-or-string per token,
same convention as aux), giving lossless round-tripping without the
added complexity of nested union-arm dispatch. Distinguishing this
from an ordinary fixed-length array column (EVT's pxdp/petm,
Array(shape=["nseg-1"])) took a real bug fix: only Array(shape=[])
(genuinely unbounded) gets the "array" treatment.
Found and fixed a real bug along the way: constructing a union-typed
item from a raw user-supplied tuple (e.g. stress_period_data={0:
[(0, "STATUS", "ACTIVE")]}) was dispatching through Item.from_tokens,
which assumes 1-based raw file tokens -- double-adjusting an
already-0-based index. New construct_union_item() dispatches by
keyword the same way, then builds the arm positionally (like
construct_item) instead.
3. Finished a `path()`/`xattree_path()` parameter rename from
`inout: Literal["filein", "fileout"]` to `direction: Literal["in",
"out"]` that was left half-applied (mismatched metadata keys between
spec.py/item.py/unstructure.py/filters.py, some files with the key
renamed but not the values) -- unrelated to the above, bundled here
because both changes require the same regeneration pass.
Updated all `save_head=`/`save_budget=`/etc. call sites across the test
suite to the new stress_period_data representation, and the handful of
tests asserting the old generic placeholder's `.number`/`.keyword`/
`.value` field names to the new per-arm typed field names. Full test
suite matches the pre-existing baseline exactly (523 passed, 21
pre-existing failures needing an mf6 binary this environment doesn't
have, 12 skipped, 1 xfailed); ruff and mypy clean.
Not done: flopy4/mf6/adapters.py and docs/examples/* still reference the
old save_head=/print_budget= API in a few places -- adapters.py's usage
was already generic/unaffected (comment updated only), but the example
scripts and notebooks need a follow-up pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017SvRiB5PxAt8PQmo8m4bTw
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SvRiB5PxAt8PQmo8m4bTw
…t_budget= API
Same conversion as the test suite: save_head=/save_budget=/print_budget=
kwargs -> stress_period_data={kper: [("SAVE"|"PRINT", rtype, setting), ...]}.
.ipynb notebooks regenerated from their .py jupytext sources via
`jupytext --sync` rather than hand-edited. frenchman-flat.ipynb picked up
an unrelated, larger diff in the process (Wel's q={...} -> stress_period_data=
list-of-tuples) -- that was already the .py source's content pre-dating this
change; the notebook just hadn't been synced since.
Verified all four docs/examples scripts (quickstart, circle, twri,
frenchman-flat) construct and sim.write() successfully with the new OC
API -- they only fail at the actual mf6 binary invocation, same
pre-existing sandbox limitation as everywhere else. Spot-checked the
written .oc files' MF6 syntax directly (SAVE/PRINT HEAD/BUDGET tokens,
STEPS/LAST settings) -- correct.
docs/examples/quickstart_expanded.py left untouched: its docstring frames
it as a design-exploration document ("we explore a few options... some
which would probably need DFN changes to support"), it's explicitly
excluded from test/test_examples.py, and its one active code line
already used a different, never-implemented perioddata= kwarg rather
than the old save_head= API -- there's no real call site there to
migrate, just historical design commentary.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017SvRiB5PxAt8PQmo8m4bTw
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.
mostly to make the Python classes a closer structural match to the DFNs
inout→directionand corresponding literals