diff --git a/autotest/dfns/__snapshots__/v2.0.0.dev2/gwf-sfr.json b/autotest/dfns/__snapshots__/v2.0.0.dev2/gwf-sfr.json index 3065c9b1..628d2a14 100644 --- a/autotest/dfns/__snapshots__/v2.0.0.dev2/gwf-sfr.json +++ b/autotest/dfns/__snapshots__/v2.0.0.dev2/gwf-sfr.json @@ -401,7 +401,8 @@ "optional": true, "tagged": false, "dtype": "integer", - "index": true, + "index": "signed", + "fk": "packagedata.ifno", "shape": [ "packagedata.ncon(ifno)" ] diff --git a/autotest/dfns/__snapshots__/v2.0.0.dev2/gwf-sfr.toml b/autotest/dfns/__snapshots__/v2.0.0.dev2/gwf-sfr.toml index bc1d2ecb..0ffba102 100644 --- a/autotest/dfns/__snapshots__/v2.0.0.dev2/gwf-sfr.toml +++ b/autotest/dfns/__snapshots__/v2.0.0.dev2/gwf-sfr.toml @@ -365,7 +365,8 @@ description = "integer value that defines the reach number of the reach connecte optional = true tagged = false dtype = "integer" -index = true +index = "signed" +fk = "packagedata.ifno" shape = [ "packagedata.ncon(ifno)", ] diff --git a/autotest/dfns/__snapshots__/v2.0.0.dev2/gwf-sfr.yaml b/autotest/dfns/__snapshots__/v2.0.0.dev2/gwf-sfr.yaml index d82cb671..870e655d 100644 --- a/autotest/dfns/__snapshots__/v2.0.0.dev2/gwf-sfr.yaml +++ b/autotest/dfns/__snapshots__/v2.0.0.dev2/gwf-sfr.yaml @@ -424,7 +424,8 @@ blocks: optional: true tagged: false dtype: integer - index: true + index: signed + fk: packagedata.ifno shape: - packagedata.ncon(ifno) diversions: diff --git a/autotest/dfns/__snapshots__/v2.0.0.dev3/gwf-sfr.json b/autotest/dfns/__snapshots__/v2.0.0.dev3/gwf-sfr.json index 5d2b6c4f..32d69479 100644 --- a/autotest/dfns/__snapshots__/v2.0.0.dev3/gwf-sfr.json +++ b/autotest/dfns/__snapshots__/v2.0.0.dev3/gwf-sfr.json @@ -406,7 +406,8 @@ "optional": true, "tagged": false, "dtype": "integer", - "index": true, + "index": "signed", + "fk": "packagedata.ifno", "shape": [ "packagedata.ncon(ifno)" ] diff --git a/autotest/dfns/__snapshots__/v2.0.0.dev3/gwf-sfr.toml b/autotest/dfns/__snapshots__/v2.0.0.dev3/gwf-sfr.toml index 14878ae4..ac786e67 100644 --- a/autotest/dfns/__snapshots__/v2.0.0.dev3/gwf-sfr.toml +++ b/autotest/dfns/__snapshots__/v2.0.0.dev3/gwf-sfr.toml @@ -368,7 +368,8 @@ description = "integer value that defines the reach number of the reach connecte optional = true tagged = false dtype = "integer" -index = true +index = "signed" +fk = "packagedata.ifno" shape = [ "packagedata.ncon(ifno)", ] diff --git a/autotest/dfns/__snapshots__/v2.0.0.dev3/gwf-sfr.yaml b/autotest/dfns/__snapshots__/v2.0.0.dev3/gwf-sfr.yaml index 4717b819..5038171d 100644 --- a/autotest/dfns/__snapshots__/v2.0.0.dev3/gwf-sfr.yaml +++ b/autotest/dfns/__snapshots__/v2.0.0.dev3/gwf-sfr.yaml @@ -427,7 +427,8 @@ blocks: optional: true tagged: false dtype: integer - index: true + index: signed + fk: packagedata.ifno shape: - packagedata.ncon(ifno) diversions: diff --git a/autotest/dfns/test_migrate.py b/autotest/dfns/test_migrate.py index 9cd0f197..ec04eaf5 100644 --- a/autotest/dfns/test_migrate.py +++ b/autotest/dfns/test_migrate.py @@ -326,3 +326,18 @@ def test_migrate_marks_cellids(dfn_dir, name, block, cols): for col in cols: field = lst.item.fields[col] assert (field.shape, field.index, field.cellid) == (["ncelldim"], True, True) + + +def test_migrate_sfr_ic_is_signed_index(dfn_dir): + component = _migrate_dev3(dfn_dir, "gwf-sfr") + item = component.blocks["connectiondata"].fields["connectiondata"].item + ic = item.fields["ic"] + assert ic.index == "signed" + assert ic.fk == "packagedata.ifno" + # nothing else in the corpus supports negative indices + signed = [ + f.name + for f in component.get_fields(recurse=True).values(multi=True) + if getattr(f, "index", False) == "signed" + ] + assert signed == ["ic"] diff --git a/autotest/dfns/test_schema.py b/autotest/dfns/test_schema.py index 4b39390f..db2809c7 100644 --- a/autotest/dfns/test_schema.py +++ b/autotest/dfns/test_schema.py @@ -1050,3 +1050,22 @@ def test_render_excludes_developmode_fields_by_default(dev3_spec): assert "CRS" not in render render_dev = dev3_spec.components["gwf-dis"].blocks["options"].render(developmode=True) assert "CRS " in render_dev + + +@pytest.mark.parametrize("index", [False, True, "signed"]) +def test_index_round_trips(index): + for field in ( + Integer(name="ic", index=index), + Array(name="ic", dtype="integer", shape=["ncon"], index=index), + ): + assert type(field).model_validate(field.model_dump()).index == index + + +def test_index_rejects_unknown_kind(): + with pytest.raises(ValueError): + Integer(name="ic", index="unsigned") + + +def test_signed_index_requires_integer_array(): + with pytest.raises(ValueError, match="requires dtype='integer'"): + Array(name="ic", dtype="double", shape=["ncon"], index="signed") diff --git a/docs/md/dfnspec.md b/docs/md/dfnspec.md index 2f902cf7..4fc64d62 100644 --- a/docs/md/dfnspec.md +++ b/docs/md/dfnspec.md @@ -390,7 +390,9 @@ Type `integer`. ###### `index` -`boolean (default: false)`. Marks this scalar's value as a 1-based index requiring MF6's 1-based (file) <-> 0-based (Python) translation on read/write. A pure serialization fact, orthogonal to `pk`/`fk`: it does not claim the value identifies or points at a list row, only that it needs the numeric-base conversion. Not valid on `String` scalars — such fields (e.g. dynamic, name-resolved identifiers) never need this conversion. +`boolean | "signed" (default: false)`. Marks this scalar's value as a 1-based index requiring MF6's 1-based (file) <-> 0-based (Python) translation on read/write. A pure serialization fact, orthogonal to `pk`/`fk`: it does not claim the value identifies or points at a list row, only that it needs the numeric-base conversion. Not valid on `String` scalars — such fields (e.g. dynamic, name-resolved identifiers) never need this conversion. + +`"signed"` marks a signed index: the value's magnitude is the 1-based index, and its sign carries a separate meaning, which only the field's description defines. There is no 0-based signed equivalent (index 0 has no negative), so a consumer must keep the sign apart from the magnitude rather than translate the value as a whole. A `fk` on a signed index applies to the magnitude. ###### `pk` @@ -460,11 +462,11 @@ Dimensions are listed fastest-varying first, i.e. in the order elements are read ###### `index` -`boolean (default: false)`. Marks the array's elements as 1-based indices requiring MF6's 1-based (file) <-> 0-based (Python) translation (e.g. `icvert`, `ja`, `irch`, `ievt`). Only valid when `dtype` is `"integer"`. +`boolean | "signed" (default: false)`. Marks the array's elements as 1-based indices requiring MF6's 1-based (file) <-> 0-based (Python) translation (e.g. `icvert`, `ja`, `irch`, `ievt`). `"signed"` marks each element as a signed index, as for a scalar (e.g. SFR's `ic`). Only valid when `dtype` is `"integer"`. ###### `fk` -`string | null (default: null)`. Marks the array's (nonzero) elements as a foreign key: a per-grid-cell reference to a row (by `pk`) in another list, rather than the per-list-row reference a scalar `fk` expresses (e.g. a grid-wide array giving each cell's cross-section id, referencing the cross-section package's `packagedata`). Only valid when `dtype` is `"integer"`. Hierarchical path form only (`"[component.]block.field"`) — an array has no `fk_ref` counterpart, since it has no sibling record to carry a runtime component-selector field, and no `pk` counterpart, since it has no rows of its own to be a key of. See "Primary/foreign keys". +`string | null (default: null)`. Marks the array's (nonzero) elements as a foreign key, each a reference to a row (by `pk`) in another list. On a grid array that's a per-grid-cell reference, rather than the per-list-row reference a scalar `fk` expresses (e.g. a grid-wide array giving each cell's cross-section id, referencing the cross-section package's `packagedata`). On an array column in a list item record, each row references several rows (e.g. SFR's `ic`, the reaches connected to a reach). Only valid when `dtype` is `"integer"`. Hierarchical path form only (`"[component.]block.field"`) — an array has no `fk_ref` counterpart, since it has no sibling record to carry a runtime component-selector field, and no `pk` counterpart, since it has no rows of its own to be a key of. See "Primary/foreign keys". ###### `cellid` diff --git a/modflow_devtools/dfn/schema.py b/modflow_devtools/dfn/schema.py index c2712467..8f96c7c0 100644 --- a/modflow_devtools/dfn/schema.py +++ b/modflow_devtools/dfn/schema.py @@ -77,6 +77,7 @@ class Field(TypedDict): layered: NotRequired[bool | None] preserve_case: NotRequired[bool] numeric_index: NotRequired[bool] + support_negative_index: NotRequired[bool] # Whether MF6 requires this block's header to appear in the input file # even when it has zero body lines (e.g. an empty required recarray # block). Set explicitly on the block's aggregate field -- see diff --git a/modflow_devtools/dfns/migrate_to_v2_0_0_dev2.py b/modflow_devtools/dfns/migrate_to_v2_0_0_dev2.py index 797cae7e..330a352c 100644 --- a/modflow_devtools/dfns/migrate_to_v2_0_0_dev2.py +++ b/modflow_devtools/dfns/migrate_to_v2_0_0_dev2.py @@ -577,9 +577,10 @@ def _walk(field: Any) -> Any: # Real relational facts the general `_resolve_relations`/`_mark_lonely_pk` # passes can't infer, backfilled by an explicit, audited allowlist rather than -# derived. Each entry: component -> (block name, list field name, {column: -# fk target}). Unlike `_NODE_REF_FIELDS`, block name and list field name +# derived. Each entry: component -> [(block name, list field name, {column: +# fk target}), ...]. Unlike `_NODE_REF_FIELDS`, block name and list field name # aren't assumed equal (MF6's period/perioddata idiom applies to two of these). +# A column may be an integer scalar or an integer array (each element a key). # # - UZF's `ivertcon` is a self-referential fk (a UZF cell may point to another # UZF cell below it) -- a same-block relation `_resolve_relations` @@ -587,6 +588,9 @@ def _walk(field: Any) -> Any: # - SFR's `iconr` is the downstream reach receiving diverted water -- a # reference into `packagedata`'s reach numbers, not the record's leading # field, so `_mark_lonely_pk` doesn't catch it either. +# - SFR's `ic` lists the reaches connected to the current one. It's a signed +# index (`support_negative_index`): the sign says which end of the current +# reach, the magnitude which reach, so the fk applies to the magnitude. # - `chf-cdb`/`chf-zdg`/`olf-zdg`'s `idcxs` reference the cross-section # defined by `chf-cxs`'s `packagedata.idcxs` -- a cross-component fk # `_resolve_relations` structurally can't reach (single-component scope). @@ -595,12 +599,15 @@ def _walk(field: Any) -> Any: # cross-section object, so the fk target is `chf-cxs`. `chf/olf-dfw`'s # `idcxs` is the same relation but is an `Array` field, not a list column # -- see `_ARRAY_FK_BACKFILL` below, not here. -_FK_BACKFILL: dict[str, tuple[str, str, dict[str, str]]] = { - "gwf-uzf": ("packagedata", "packagedata", {"ivertcon": "packagedata.ifno"}), - "gwf-sfr": ("diversions", "diversions", {"iconr": "packagedata.ifno"}), - "chf-cdb": ("period", "stress_period_data", {"idcxs": "chf-cxs.packagedata.idcxs"}), - "chf-zdg": ("period", "stress_period_data", {"idcxs": "chf-cxs.packagedata.idcxs"}), - "olf-zdg": ("period", "stress_period_data", {"idcxs": "chf-cxs.packagedata.idcxs"}), +_FK_BACKFILL: dict[str, list[tuple[str, str, dict[str, str]]]] = { + "gwf-uzf": [("packagedata", "packagedata", {"ivertcon": "packagedata.ifno"})], + "gwf-sfr": [ + ("diversions", "diversions", {"iconr": "packagedata.ifno"}), + ("connectiondata", "connectiondata", {"ic": "packagedata.ifno"}), + ], + "chf-cdb": [("period", "stress_period_data", {"idcxs": "chf-cxs.packagedata.idcxs"})], + "chf-zdg": [("period", "stress_period_data", {"idcxs": "chf-cxs.packagedata.idcxs"})], + "olf-zdg": [("period", "stress_period_data", {"idcxs": "chf-cxs.packagedata.idcxs"})], } @@ -610,28 +617,26 @@ def _apply_fk_backfill(name: str, blocks: dict[str, v2.Block]) -> dict[str, v2.B See `_FK_BACKFILL` for why this is an explicit allowlist rather than a derived/mechanical pass. """ - entry = _FK_BACKFILL.get(name) - if entry is None: - return blocks - block_name, list_field_name, field_targets = entry - block = blocks.get(block_name) - if block is None: - return blocks - list_field = block.fields.get(list_field_name) - if not isinstance(list_field, v2.List) or not isinstance(list_field.item, v2.Record): - return blocks - item = list_field.item - updates = { - fname: f.model_copy(update={"fk": target}) - for fname, target in field_targets.items() - if isinstance(f := item.fields.get(fname), v2.Integer) and f.fk is None - } - if not updates: - return blocks - new_item = item.model_copy(update={"fields": {**item.fields, **updates}}) - new_list = list_field.model_copy(update={"item": new_item}) - new_block = block.model_copy(update={"fields": {**block.fields, list_field_name: new_list}}) - return {**blocks, block_name: new_block} + for block_name, list_field_name, field_targets in _FK_BACKFILL.get(name, []): + block = blocks.get(block_name) + if block is None: + continue + list_field = block.fields.get(list_field_name) + if not isinstance(list_field, v2.List) or not isinstance(list_field.item, v2.Record): + continue + item = list_field.item + updates = { + fname: f.model_copy(update={"fk": target}) + for fname, target in field_targets.items() + if isinstance(f := item.fields.get(fname), v2.Integer | v2.Array) and f.fk is None + } + if not updates: + continue + new_item = item.model_copy(update={"fields": {**item.fields, **updates}}) + new_list = list_field.model_copy(update={"item": new_item}) + new_block = block.model_copy(update={"fields": {**block.fields, list_field_name: new_list}}) + blocks = {**blocks, block_name: new_block} + return blocks # `chf-dfw`/`olf-dfw`'s `idcxs` is a per-cell grid array (`griddata` block, @@ -1455,8 +1460,11 @@ def _map_field(f: v1.Field) -> v2.InputField: layered: bool = try_parse_bool(f.get("layered"), False) # Pure serialization fact (1-based/0-based conversion), orthogonal to pk/fk # relational identity. Direct copy from v1: valid on Integer/Array(dtype= - # "integer") only, never on String. - numeric_index: bool = try_parse_bool(f.get("numeric_index"), False) + # "integer") only, never on String. v1's `support_negative_index` (only + # SFR's `ic`) means the sign carries meaning and the magnitude is the index. + numeric_index: v2.Index = try_parse_bool(f.get("numeric_index"), False) + if numeric_index and try_parse_bool(f.get("support_negative_index"), False): + numeric_index = "signed" removed: str | None = f.get("removed") or None deprecated: str | None = f.get("deprecated") or None valid = f.get("valid") diff --git a/modflow_devtools/dfns/schema.json b/modflow_devtools/dfns/schema.json index f99fbfd7..f0724fcc 100644 --- a/modflow_devtools/dfns/schema.json +++ b/modflow_devtools/dfns/schema.json @@ -123,9 +123,17 @@ "type": "boolean" }, "index": { + "anyOf": [ + { + "type": "boolean" + }, + { + "const": "signed", + "type": "string" + } + ], "default": false, - "title": "Index", - "type": "boolean" + "title": "Index" }, "fk": { "anyOf": [ @@ -635,9 +643,17 @@ "type": "boolean" }, "index": { + "anyOf": [ + { + "type": "boolean" + }, + { + "const": "signed", + "type": "string" + } + ], "default": false, - "title": "Index", - "type": "boolean" + "title": "Index" }, "pk": { "default": false, diff --git a/modflow_devtools/dfns/schema.py b/modflow_devtools/dfns/schema.py index f8362003..3b812dbe 100644 --- a/modflow_devtools/dfns/schema.py +++ b/modflow_devtools/dfns/schema.py @@ -90,11 +90,17 @@ class String(InputFieldBase): fk_ref: str | None = None +# `True`: a 1-based index in the file, 0-based in memory. `"signed"`: the +# magnitude is such an index, and the sign carries a meaning of its own, which +# only the field's description defines (e.g. SFR's `ic`). +Index = bool | Literal["signed"] + + class Integer(InputFieldBase): type: Literal["integer"] = PydanticField(default="integer", frozen=True) valid: list[int] | None = None time_series: bool = False - index: bool = False + index: Index = False pk: bool = False fk: str | None = None fk_ref: str | None = None @@ -122,7 +128,7 @@ class Array(InputFieldBase): shape: list[str] = [] time_series: bool = False layered: bool = False - index: bool = False + index: Index = False fk: str | None = None # A cellid, or several: a grid cell reference resolved against the grid # the column refers to (DIS/DISV/DISU). The first (fastest-varying) axis @@ -133,7 +139,8 @@ class Array(InputFieldBase): def _check_index_dtype(self) -> "Array": if self.index and self.dtype != "integer": raise ValueError( - f"Array {self.name!r}: index=True requires dtype='integer', got {self.dtype!r}" + f"Array {self.name!r}: index={self.index!r} requires dtype='integer', " + f"got {self.dtype!r}" ) if self.cellid and self.dtype != "integer": raise ValueError(