fix(dfns): multiple fixes/improvements - #349
Merged
Merged
Conversation
v1 DFN syntax allows a boolean attribute line with no value at all (e.g. a bare `optional` line, as opposed to `optional true`/`optional false`), meaning the attribute is set. `load_dfn` parses such a line to an empty string (as opposed to `None` for a wholly absent attribute), but `try_parse_bool` treated "" as `False` unconditionally, silently inverting the flag regardless of the caller's default. Fixed at the root in `try_parse_bool` rather than patching call sites individually, since the bug affects every boolean DFN attribute that uses the bare-line convention, not just `optional`. Real-corpus impact turned out wider than the printrecord family originally scoped: chf-dfw, chf-disv1d, gwf-npf, olf-dfw, olf-disv1d, swf-dfw, swf-disv1d, and sim-nam also had bare-`optional` fields now migrated correctly. Updates one existing test that had the old buggy rendering of gwf-oc's columns/width/digits baked in as required; they render bracketed as optional now, matching flopy4's independently-confirmed real MF6 behavior for the same fields. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Array.repeat was added in the v2 schema rewrite (MODFLOW-ORG#311) as a `str | None` field-name reference, replacing v1's plain `repeating: bool`, but the migration was never wired up to populate it, and the spec's promised elaboration ("See `repeat` section below") was never written -- no such section exists. Checked what the 3 real corpus fields with v1 `repeating: true` (utl-tas.tas_array, prt-oc.times, prt-prp.times) would even reference: all 3 also have v1 `shape (any1d)`, which the mapper already collapses to `shape=[]` (self-sizing), matching dfnspec.md's existing self-sizing semantics exactly. None has a sibling field to point to as a count source, so the reference design never matched real data. Removing rather than backfilling a migration for an attribute nothing consumes and no data supports. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fixup _mark_lonely_pk matched a block's list field by the *block's own* name (gated to a "packagedata"/"perioddata" allowlist), but MF6's convention names the block "period" and its list field "perioddata" -- the two are never the same string, so the "perioddata" entry in the allowlist could never match any component. Fixed by matching on the list field's own name instead (_LONELY_PK_FIELDS). Confirmed via corpus scan: this was dead for every package, not just the gwf-mvr case the old docstring called out; utl-spc.bndno is the first real catch. This also required moving _mark_lonely_pk's call to after _fix_lak_relations/_fix_mvr_relations: LAK's period `number` field (the deliberately-ambiguous lake-or-outlet field split by _fix_lak_relations) structurally looks exactly like a lonely pk before that split runs, and _fix_lak_relations builds its lakeno/outletno fk copies via number_field.model_copy(...), which would carry a stale pk=True into fields that are actually foreign keys. Verified no other pass between the old and new call sites reads pk/fk state. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…te plan Adds todo.md (working log of the v1->v2 migration gaps found during the flopy4 dev3 migration, including the fixes in this branch and a corpus-scan breakdown of the remaining numeric_index -> pk/fk gap into five structurally distinct sub-issues) and index-node-attributes-plan.md (implementation plan for splitting the pk/fk vocabulary's conflated "needs 1-based/0-based conversion" and "is a relational key" concerns into dedicated `index` and `node` attributes, with a phased sequence and the flopy4-side coordination this requires). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Splits the numeric_index -> pk/fk gap's serialization concern out of the relational pk/fk vocabulary, per index-node-attributes-plan.md Phase 1. Adds Integer.index and Array.index (dtype="integer" only, enforced by a validator; never valid on String) to schema.py, documents both in dfnspec.md, regenerates schema.json (also picking up pre-existing unrelated drift: removed/deprecated/layered were missing from Array's schema), and migrates index directly from v1 numeric_index in migrate_to_v2_0_0_dev2.py for every Integer/Array(dtype="integer") field. Confirmed this closes the gwf-lak.iconn regression tracked in todo.md (MF6 rejected the file without the 1-based/0-based conversion index=True provides) and coexists correctly with existing pk/fk (e.g. chf-cxs.idcxs keeps pk=true and gains index=true). Full autotest/dfns/ snapshot suite regenerated; diff is additive-only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DTmmUhFmfsjihggQUcZ3Ja
Splits the grid-cell-reference concern out of pk/fk entirely, per index-node-attributes-plan.md Phase 2. Adds Integer.node to schema.py (documentation-level placement convention, same as pk/fk — no schema validator, mirroring how pk/fk placement is already unenforced); removes _validate_fk_fields's fk == "node" exemption so it's a single hierarchical-path-vs-fk_ref invariant check — an unqualified fk="node" is no longer special-cased and must resolve to a real list block like any other bare fk value, which structurally deprecates it without a hard reject. Updates dfnspec.md (node documented under Integer, "Primary and foreign keys" grid-cell-sentinel form replaced with a pointer to node) and regenerates schema.json. Backfills the 6 known corpus candidates (5 exchange components' exchangedata.cellidm1/cellidm2, gwf-gnc's gncdata.cellidm/cellidn) via a new _mark_node_refs pass in migrate_to_v2_0_0_dev2.py, keyed on an explicit _NODE_REF_FIELDS allowlist — audited per field rather than derived, since v1 has no attribute this maps from mechanically. test_schema_relations.py's node-sentinel-specific rejection test replaced with one confirming the new (unremarkable) behavior. Full autotest/dfns/ suite green; snapshots regenerated, additive-only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DTmmUhFmfsjihggQUcZ3Ja
Completes the pk/fk backfill items from index-node-attributes-plan.md Phase 3 (todo.md gap #2 sub-issues (b)/(d)/(f)), now that pk/fk no longer double as the serialization signal (index/node cover that). - _LONELY_PK_FIELDS widened to vertices/cell2d/cell1d: DISU/DISV/ DISV1D/DISV2D's leading iv/icell2d/icell1d columns are genuinely the same lonely-pk shape _mark_lonely_pk already detects (12 components). - New _apply_fk_backfill pass (mirrors Phase 2's _mark_node_refs) sets fk on three real single-column relations the general passes can't infer: SFR diversions.iconr -> packagedata.ifno (a real reference into reach numbers, confirmed from its description -- not a compound-scoped id like its sibling idv, which correctly keeps index only), UZF packagedata.ivertcon -> packagedata.ifno (a self-referential relation: a UZF cell may point to another UZF cell in the same list), and idcxs on chf-cdb/chf-zdg/olf-zdg/swf-zdg -> the matching *-cxs component's packagedata.idcxs (cross-component, which _resolve_relations is structurally single-component-scoped and can't reach). - Cross-component fk backfill surfaced a latent validator bug: _validate_fk_fields's fk.split(".")[0] only ever read the first dot-segment as the block name, so the documented 3-segment "component.block.field" form would have been misparsed and rejected -- nobody had hit it before because no fk had ever actually used that form. Fixed to resolve the target component via spec.components when 3 segments are present. Investigated and deliberately left alone (documented in todo.md/ index-node-attributes-plan.md rather than silently dropped): MAW's connectiondata.icon (same compound-scoped, no-real-target shape as gwf-lak.iconn -- index only is correct) and chf/olf/swf-dfw's idcxs (an Array, shape=["nodes"], which can't carry fk under the current schema at all). All five decomposed sub-issues from the original numeric_index -> pk/fk gap scan (a)-(g) are now resolved. Full autotest/dfns/ suite green; snapshots regenerated, additive-only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DTmmUhFmfsjihggQUcZ3Ja
Follow-up to the Phase 3 pk/fk backfill commit, which left two cases deliberately alone: MAW's connectiondata.icon (no real target -- same shape as gwf-lak.iconn, index alone is correct and complete, reconfirmed here rather than changed) and chf/olf/swf-dfw's idcxs (a per-cell grid Array referencing the matching *-cxs component's packagedata.idcxs, which couldn't be expressed because Array had no fk attribute at all). The idcxs case turned out to be a real, closable gap rather than a permanent schema limitation: adds Array.fk (dtype="integer" only, via the same validator index already uses) with no pk/fk_ref counterpart (an array has no rows of its own to key, and no sibling record for a runtime component selector). _validate_fk_fields already validates it for free -- its field walk reads fk/fk_ref via getattr regardless of type, so no validator change was needed beyond the attribute existing. Backfills fk = "<component>-cxs.packagedata.idcxs" on all 3 *-dfw components via a new _apply_array_fk_backfill pass. Updates dfnspec.md (Array's fk documented; "Primary and foreign keys" scope note widened) and regenerates schema.json. Full autotest/dfns/ suite green; snapshots regenerated (18 files, additive-only). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DTmmUhFmfsjihggQUcZ3Ja
…control Working notes for the dfns index/node/pk/fk migration plan, now that all phases described in them are complete. Kept locally, untracked, rather than deleted outright. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DTmmUhFmfsjihggQUcZ3Ja
…schema.py comments try_parse_bool is only used by the v2 dfn migration code, so move it out of the generic misc module and into migrate_to_v2_0_0_dev2 (dev3 already imports from that module). Also drop docstrings/comments on private schema.py helpers that mostly restated the code, and rename _can_share_model to _receives_from with requester/provider-named params to make the direction of the model-scope dim-visibility check explicit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PCFraGPS9DuhabpEnreYUy
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.
Migration fixes, schema improvements, and cleanup motivated by migrating the flopy4 prototype to the latest schema
try_parse_boolconsidered no value to be false instead of trueArray.repeatattribute_can_share_model->_receives_from