Skip to content

Commit 933a6e6

Browse files
committed
Require explicit item and z_item on VerticalObservation / VerticalModelResult
Drops the positional defaults (`z_item=0`, `item=None` with 2-column auto-pick). A vertical profile's depth axis and value column are the defining inputs; defaulting them to "column 0" / "column 1" is a position-based assumption in an API that should be name-based. The same flaw exists on TrackObservation and TrackModelResult but those have real users and will get a separate deprecation PR; vertical is alpha and can break cleanly. Internal transforms (`sel`, `trim`, etc.) reach the constructors via the base `_create_new_instance` which calls `self.__class__(data)` with no kwargs. Two overrides reconstruct `item` from `self.name` and use `z_item="z"` (always renamed by the parser), preserving the prevalidated- dataset path without leaking sentinel defaults into the public signature. Two other internal call sites that bypassed `_create_new_instance` (`comparison/_comparison.py` and `comparison/_vertical_comparison.py`) now go through it for consistency. Also fixes a docstring typo (`mikeio.Dfs0, mikeio.Dfs0` -> `mikeio.Dfs0, mikeio.Dataset`) and switches both data-arg docstrings from type-listing to semantic description.
1 parent ba4024e commit 933a6e6

8 files changed

Lines changed: 84 additions & 72 deletions

File tree

src/modelskill/comparison/_comparison.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -948,7 +948,7 @@ def sel(
948948
m_data = v.data.where(
949949
(v.data["z"] >= lo) & (v.data["z"] <= hi), drop=True
950950
)
951-
raw_mod[k] = type(v)(m_data) # type: ignore[call-arg]
951+
raw_mod[k] = v._create_new_instance(m_data)
952952
raw_mod_data = raw_mod
953953
else:
954954
z_mask = xr.apply_ufunc(np.isclose, d["z"], float(z))

src/modelskill/comparison/_vertical_comparison.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -366,4 +366,4 @@ def _raw_model_to_z(self, raw_mod, z):
366366
z_dist.reset_index().groupby("time", sort=False)["z"].idxmin().to_numpy()
367367
)
368368
sel_data = raw_mod.data.isel(time=np.sort(nearest_idx))
369-
return type(raw_mod)(sel_data)
369+
return raw_mod._create_new_instance(sel_data)

src/modelskill/model/vertical.py

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
from __future__ import annotations
22
from typing import Any, Sequence
3+
from typing_extensions import Self
34

45
import xarray as xr
56
import pandas as pd
@@ -19,16 +20,17 @@ class VerticalModelResult(TimeSeries):
1920
2021
Parameters
2122
----------
22-
data : str, Path, pd.DataFrame, mikeio.Dfs0, mikeio.Dfs0, xr.Dataset
23-
The input data or file path
23+
data : dfs0 path or in-memory profile data
24+
Path to a dfs0 file, or a long-format DataFrame / mikeio.Dataset /
25+
xr.Dataset with a time index, a vertical-coordinate column, and one
26+
or more value columns.
27+
item : str | int
28+
Index or name of the primary model item.
29+
z_item : str | int
30+
Index or name of the vertical coordinate item.
2431
name : str | None, optional
2532
The name of the model result,
2633
by default None (will be set to file name or item name)
27-
item : str | int | None, optional
28-
If multiple items/arrays are present in the input an item
29-
must be given (as either an index or a string), by default None
30-
z_item : str | int | None, optional
31-
Item of the first coordinate of positions, by default None
3234
x : float, optional
3335
lateral coordinate of point position, inferred from data if not given, else None
3436
y : float, optional
@@ -43,10 +45,10 @@ def __init__(
4345
self,
4446
data: VerticalType,
4547
*,
48+
item: str | int,
49+
z_item: str | int,
4650
name: str | None = None,
47-
item: str | int | None = None,
4851
quantity: Quantity | None = None,
49-
z_item: str | int = 0,
5052
x: float | None = None,
5153
y: float | None = None,
5254
aux_items: Sequence[int | str] | None = None,
@@ -72,6 +74,10 @@ def z(self) -> Any:
7274
"""z-coordinate"""
7375
return self._coordinate_values("z")
7476

77+
def _create_new_instance(self, data: xr.Dataset) -> Self:
78+
"""Reconstruct instance from a modelskill-built dataset."""
79+
return self.__class__(data, item=self.name, z_item="z")
80+
7581
def _match_to_nearest_times(
7682
self, obs_df: pd.DataFrame, t_tol: pd.Timedelta | None = None
7783
) -> pd.DataFrame:

src/modelskill/obs.py

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -374,20 +374,20 @@ class VerticalObservation(Observation):
374374
375375
Parameters
376376
----------
377-
data : (str, Path, pd.DataFrame, mikeio.Dfs0, mikeio.Dataset, xr.Dataset)
378-
Input data with vertical profile observations.
379-
item : int or str, optional
377+
data : dfs0 path or in-memory profile data
378+
Path to a dfs0 file, or a long-format DataFrame / mikeio.Dataset /
379+
xr.Dataset with a time index, a vertical-coordinate column, and one
380+
or more value columns.
381+
item : int or str
380382
Index or name of the primary observation item.
381-
If the input contains more than one candidate value item,
382-
this argument must be provided.
383+
z_item : int or str
384+
Index or name of the vertical coordinate item.
383385
x : float, optional
384386
x-coordinate of the observation location. If not provided,
385387
it is inferred from data when possible.
386388
y : float, optional
387389
y-coordinate of the observation location. If not provided,
388390
it is inferred from data when possible.
389-
z_item : int or str, optional
390-
Index or name of the vertical coordinate item, by default 0.
391391
name : str, optional
392392
User-defined name for identification in plots and summaries.
393393
weight : float, optional
@@ -437,10 +437,10 @@ def __init__(
437437
self,
438438
data: VerticalType,
439439
*,
440-
item: int | str | None = None,
440+
item: int | str,
441+
z_item: int | str,
441442
x: float | None = None,
442443
y: float | None = None,
443-
z_item: int | str | None = 0,
444444
name: str | None = None,
445445
weight: float = 1.0,
446446
quantity: Quantity | None = None,
@@ -465,6 +465,10 @@ def __init__(
465465
def z(self):
466466
return self._coordinate_values("z")
467467

468+
def _create_new_instance(self, data: xr.Dataset) -> Self:
469+
"""Reconstruct instance from a modelskill-built dataset."""
470+
return self.__class__(data, item=self.name, z_item="z")
471+
468472

469473
class NodeObservation(Observation):
470474
"""Class for observations at network nodes.

src/modelskill/timeseries/_vertical.py

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,22 +29,15 @@ def all(self) -> List[str]:
2929

3030
def _parse_vertical_items(
3131
items: Sequence[Hashable],
32-
z_item: int | str | None,
33-
item: int | str | None,
32+
z_item: int | str,
33+
item: int | str,
3434
aux_items: Optional[Sequence[int | str]] = None,
3535
) -> VerticalItem:
36-
"""If input has exactly 2 items we accept item=None"""
36+
"""Resolve and validate item selection from available column names."""
3737
if len(items) < 2:
3838
raise ValueError(
3939
f"Input has only {len(items)} items. It should have at least 2."
4040
)
41-
if item is None:
42-
if len(items) == 2:
43-
item = 1
44-
elif len(items) > 2:
45-
raise ValueError(
46-
f"Input has more than 2 items, but item was not given! Available items: {items}"
47-
)
4841

4942
item = _get_name(item, valid_names=items)
5043
z_item = _get_name(z_item, valid_names=items)
@@ -82,9 +75,9 @@ def _include_location(
8275
def _parse_vertical_input(
8376
data: VerticalType,
8477
name: Optional[str],
85-
item: str | int | None,
78+
item: str | int,
8679
quantity: Optional[Quantity],
87-
z_item: str | int | None,
80+
z_item: str | int,
8881
x: float | None = None,
8982
y: float | None = None,
9083
aux_items: Optional[Sequence[int | str]] = None,

tests/model/test_vertical.py

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -102,20 +102,21 @@ def test_open_with_factory(self, dfs0_fpath):
102102
# ================
103103
# Test failing and optional args
104104
# ================
105-
# failing without z_item
106-
def test_fail_with_3_items_no_item_arg(self, dfs0_ds):
107-
ds_test = dfs0_ds.copy()
108-
ds_test["extra_item"] = ds_test[1].copy()
109-
with pytest.raises(ValueError, match="Input has more than 2 items, but"):
110-
_ = ms.VerticalModelResult(ds_test)
111-
112-
# failing z wronge location
105+
def test_missing_item_kwarg_raises(self, dfs0_ds):
106+
with pytest.raises(TypeError, match="item"):
107+
ms.VerticalModelResult(dfs0_ds, z_item="z")
108+
109+
def test_missing_z_item_kwarg_raises(self, dfs0_ds):
110+
with pytest.raises(TypeError, match="z_item"):
111+
ms.VerticalModelResult(dfs0_ds, item="Salinity")
112+
113+
# failing z wrong location
113114
def test_item_named_z(self, dfs0_ds):
114115
ds_test = mikeio.Dataset(
115116
[dfs0_ds[1], dfs0_ds[0]],
116117
)
117118
with pytest.raises(ValueError, match="name 'z' is reserved "):
118-
_ = ms.VerticalModelResult(ds_test)
119+
_ = ms.VerticalModelResult(ds_test, item="z", z_item="Salinity")
119120

120121
def test_duplicate_time_z_pairs_raises(self):
121122
df = pd.DataFrame(
@@ -167,7 +168,7 @@ def test_vertical_model_roundtrip_from_dataset(self, vertical_model_df):
167168
y=55.0,
168169
name="salt_model",
169170
)
170-
mr2 = ms.VerticalModelResult(mr.data)
171+
mr2 = ms.VerticalModelResult(mr.data, item="Salinity", z_item="z")
171172

172173
assert mr.equals(mr2)
173174
assert mr2.gtype == mr.gtype

tests/observation/test_vertical_obs.py

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -73,28 +73,23 @@ def test_sel_by_z_scalar(self, _vertical_df):
7373
)
7474

7575
# out = obs.sel(z=-4.0) # only works with xarray >= 2026.01.0
76-
out = ms.VerticalObservation(obs.data.where(obs.data["z"] == -4.0, drop=True))
76+
out = ms.VerticalObservation(
77+
obs.data.where(obs.data["z"] == -4.0, drop=True),
78+
item="value",
79+
z_item="z",
80+
)
7781

7882
assert isinstance(out, ms.VerticalObservation)
7983
assert len(out.data) == 1
8084
assert out.data.value == 1.1
8185

8286
def test_open_dfs0_equal(self):
8387
fn = Path("tests/testdata/vertical/VerticalProfile_obs2.dfs0")
84-
obs = ms.observation(fn, z_item="z")
85-
obs2 = ms.VerticalObservation(fn)
88+
obs = ms.observation(fn, item="Salinity", z_item="z")
89+
obs2 = ms.VerticalObservation(fn, item="Salinity", z_item="z")
8690
assert isinstance(obs, ms.VerticalObservation)
8791
assert obs.equals(obs2)
8892

89-
def test_with_and_without_item_arg(self):
90-
fn = Path("tests/testdata/vertical/VerticalProfile_ST.dfs0")
91-
# no item specified, but multiple items in file
92-
with pytest.raises(ValueError):
93-
ms.observation(fn, z_item="z")
94-
# below should be fine...only one item
95-
fn = Path("tests/testdata/vertical/VerticalProfile_obs1.dfs0")
96-
assert isinstance(ms.observation(fn, z_item="z"), ms.VerticalObservation)
97-
9893
def test_duplicate_time_z_pairs_raises(self):
9994
df = pd.DataFrame(
10095
{
@@ -121,19 +116,22 @@ def test_single_item_input_raises(self):
121116
with pytest.raises(ValueError, match="at least 2"):
122117
ms.VerticalObservation(df, item="value", z_item="z", x=12.0, y=55.0)
123118

124-
def test_more_than_two_items_without_item_raises(self):
119+
def test_missing_item_kwarg_raises(self):
125120
df = pd.DataFrame(
126-
{
127-
"z": [-5.0, -4.0, -3.0],
128-
"value1": [1.0, 1.1, 1.2],
129-
"value2": [2.0, 2.1, 2.2],
130-
},
121+
{"z": [-5.0, -4.0, -3.0], "value": [1.0, 1.1, 1.2]},
131122
index=[pd.Timestamp("2019-01-01")] * 3,
132123
)
133-
134-
with pytest.raises(ValueError, match="item was not given"):
124+
with pytest.raises(TypeError, match="item"):
135125
ms.VerticalObservation(df, z_item="z", x=12.0, y=55.0)
136126

127+
def test_missing_z_item_kwarg_raises(self):
128+
df = pd.DataFrame(
129+
{"z": [-5.0, -4.0, -3.0], "value": [1.0, 1.1, 1.2]},
130+
index=[pd.Timestamp("2019-01-01")] * 3,
131+
)
132+
with pytest.raises(TypeError, match="z_item"):
133+
ms.VerticalObservation(df, item="value", x=12.0, y=55.0)
134+
137135
def test_duplicate_item_specification_raises(self, _vertical_df_aux):
138136
with pytest.raises(ValueError, match="Duplicate items"):
139137
ms.VerticalObservation(
@@ -181,7 +179,7 @@ def test_roundtrip_from_dataset_preserves_vertical_observation(self, _vertical_d
181179
y=55.0,
182180
attrs={"station": "A"},
183181
)
184-
obs2 = ms.VerticalObservation(obs.data)
182+
obs2 = ms.VerticalObservation(obs.data, item="value", z_item="z")
185183

186184
assert obs.equals(obs2)
187185
assert obs2.attrs["gtype"] == obs.attrs["gtype"]

tests/test_match.py

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import modelskill as ms
88
from modelskill.comparison._comparison import ItemSelection
99
from modelskill.model.dfsu import DfsuModelResult
10+
1011
try:
1112
from modelskill.network import _make_basic_network
1213
except ImportError:
@@ -82,12 +83,16 @@ class TestVerticalObservation:
8283
@pytest.fixture(scope="class")
8384
def o4(self):
8485
fn = "tests/testdata/vertical/VerticalProfile_obs1.dfs0"
85-
return ms.VerticalObservation(fn, z_item="z", name="vobs", x=657500, y=6553600)
86+
return ms.VerticalObservation(
87+
fn, item="Salinity", z_item="z", name="vobs", x=657500, y=6553600
88+
)
8689

8790
@pytest.fixture(scope="class")
8891
def mr4(self):
8992
fn = "tests/testdata/vertical/VerticalModel_at_obs.dfs0"
90-
return ms.model_result(fn, item="Salinity", name="vmod", gtype="vertical")
93+
return ms.model_result(
94+
fn, item="Salinity", z_item="z", name="vmod", gtype="vertical"
95+
)
9196

9297
@pytest.fixture(scope="class")
9398
def mr5(self):
@@ -262,7 +267,7 @@ def test_align_unevently_sampled_obs(self, simple_vo, simple_vm):
262267
# drop the last point and modify last obs depth to -4 and create new VerticalObservation
263268
df_o = simple_vo.to_dataframe().iloc[0:-1, :]
264269
df_o.iloc[-1, 0] = -4
265-
vo = ms.VerticalObservation(df_o)
270+
vo = ms.VerticalObservation(df_o, item=simple_vo.name, z_item="z")
266271
cmp = ms.match(vo, simple_vm)
267272
expected_mod_values = [1.1, 2.1, 4.2]
268273
expected_depths = simple_vo.data["z"].to_numpy().copy()[0:-1]
@@ -273,15 +278,17 @@ def test_align_unevently_sampled_obs(self, simple_vo, simple_vm):
273278

274279
def test_align_vertical_intepolation(self, simple_vm):
275280
# Create observations from model df
276-
vo = ms.VerticalObservation(simple_vm.to_dataframe())
281+
vo = ms.VerticalObservation(
282+
simple_vm.to_dataframe(), item=simple_vm.name, z_item="z"
283+
)
277284
# Modify model to be inbetween the obs depths and create new VerticalModelResult
278285
df = simple_vm.to_dataframe()
279286
df["z"] = df["z"] - 0.5
280287
# INPUT
281288
# mod_z = [-1.5, -2.5, -3.5, -4.5, -1.5, -2.5, -3.5, -4.5],
282289
# mod_v = [1.1, 2.1, 3.1, 4.1, 1.2, 2.2, 3.2, 4.2]
283290
# obs_z = [-1, -2, -3, -4, -1, -2, -3, -4]
284-
vm = ms.VerticalModelResult(df)
291+
vm = ms.VerticalModelResult(df, item=simple_vm.name, z_item="z")
285292
cmp = ms.match(vo, vm)
286293

287294
# first depth is nan in models becasuse obs outside model domain
@@ -292,7 +299,9 @@ def test_align_vertical_intepolation(self, simple_vm):
292299
assert cmp.data["mod"].to_numpy() == pytest.approx(expected_mod_values)
293300

294301
def test_same_results(self, simple_vm):
295-
vo = ms.VerticalObservation(simple_vm.to_dataframe())
302+
vo = ms.VerticalObservation(
303+
simple_vm.to_dataframe(), item=simple_vm.name, z_item="z"
304+
)
296305
cmp = ms.match(vo, simple_vm)
297306
assert cmp.n_points == 8
298307
assert cmp.data["mod"].to_numpy() == pytest.approx(
@@ -323,15 +332,15 @@ def test_no_overlap_in_z(self, simple_vo, simple_vm):
323332
# shift model to be outside obs range
324333
df = simple_vm.to_dataframe()
325334
df["z"] = df["z"] - 2
326-
vm = ms.VerticalModelResult(df)
335+
vm = ms.VerticalModelResult(df, item=simple_vm.name, z_item="z")
327336
cmp = ms.match(simple_vo, vm)
328337
assert cmp.n_points == 0
329338

330339
def test_only_1_model_depth_overlap(self, simple_vo, simple_vm):
331340
# shift model to be outside obs range except for one point
332341
df = simple_vm.to_dataframe()
333342
df["z"] = df["z"] - 1 # only match with models at z=-2 exact
334-
vm = ms.VerticalModelResult(df)
343+
vm = ms.VerticalModelResult(df, item=simple_vm.name, z_item="z")
335344
cmp = ms.match(simple_vo, vm)
336345
assert cmp.n_points == 2
337346

@@ -1044,7 +1053,8 @@ def test_network_match_multi_obs_multi_model_comprehensive(
10441053
def test_network_match_error_non_node_observation(network_mr, point_obs_error):
10451054
"""Test that non-NodeObservation raises appropriate error"""
10461055
with pytest.raises(
1047-
TypeError, match="NetworkModelResult supports NodeObservation and ReachObservation"
1056+
TypeError,
1057+
match="NetworkModelResult supports NodeObservation and ReachObservation",
10481058
):
10491059
ms.match(point_obs_error, network_mr)
10501060

0 commit comments

Comments
 (0)