From 9364c83a275df778b84440acf9e0b8c257c9734e Mon Sep 17 00:00:00 2001 From: jpalm3r Date: Fri, 24 Jul 2026 15:24:28 +0200 Subject: [PATCH 01/10] Commit failing test --- tests/test_network.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/test_network.py b/tests/test_network.py index 931860102..ff734393a 100644 --- a/tests/test_network.py +++ b/tests/test_network.py @@ -584,6 +584,25 @@ def test_dataframe_from_partial_network(): assert set(nodes_in_df) == set([network.find(n) for n in selected_nodes]) +@pytest.mark.skipif( + sys.version_info >= (3, 14), reason="mikeio1d requires Python < 3.14" +) +def test_nodes_filtered_network_keeps_datetime_index(): + """Topology-only nodes must not degrade the time index to object dtype. + + A nodes-filtered network keeps the full topology, storing empty data for + the unselected nodes. Concatenating those empty (RangeIndex) frames must + not corrupt the DatetimeIndex, otherwise ms.match() later fails with + "time must be datetime". See gh #676. + """ + path_to_file = "./tests/testdata/network.res1d" + network = Network.from_res1d(path_to_file, nodes=["108", "101"], reaches=[]) + + assert isinstance(network._df.index, pd.DatetimeIndex) + assert network._df.index.dtype == "datetime64[ns]" + assert network.to_dataset()["time"].dtype == np.dtype("datetime64[ns]") + + @pytest.mark.skipif( sys.version_info >= (3, 14), reason="mikeio1d requires Python < 3.14" ) From d391b80999382081f8063d31ad0204a7ed018082 Mon Sep 17 00:00:00 2001 From: jpalm3r Date: Fri, 24 Jul 2026 15:26:43 +0200 Subject: [PATCH 02/10] Filter empty node data before concat in _build_dataframe Topology-only nodes store an empty DataFrame (not None), so the existing `is not None` check never dropped them. Concatenating an empty RangeIndex frame alongside real DatetimeIndex frames degraded the result index to object dtype, which later failed the "time must be datetime" assertion in ms.match(). Also skip empty frames so the DatetimeIndex is preserved. Fixes #676 Co-Authored-By: Claude Opus 4.8 --- src/modelskill/network.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/modelskill/network.py b/src/modelskill/network.py index 071bc01f6..8d8644fe2 100644 --- a/src/modelskill/network.py +++ b/src/modelskill/network.py @@ -497,7 +497,9 @@ def _generate_reaches_dict(reaches: Sequence[NetworkReach]) -> dict[str, Network @staticmethod def _build_dataframe(g: nx.Graph) -> pd.DataFrame: data_in_nodes = { - k: v["data"] for k, v in g.nodes.items() if v["data"] is not None + k: v["data"] + for k, v in g.nodes.items() + if v["data"] is not None and not v["data"].empty } if len(data_in_nodes) == 0: columns = pd.MultiIndex.from_arrays([[], []], names=["node", "quantity"]) From fe4be00d64221acb488818b747e072f3763e3f87 Mon Sep 17 00:00:00 2001 From: jpalm3r Date: Fri, 24 Jul 2026 15:49:09 +0200 Subject: [PATCH 03/10] Add failing test for node-gtype save/load round trip ComparerCollection.save()/load() doesn't support the NODE geometry type: Comparer.load() has no "node" branch and falls into NotImplementedError("Unknown gtype: node"). Refs gh #677 --- tests/test_comparercollection.py | 41 ++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/test_comparercollection.py b/tests/test_comparercollection.py index f884ff7f4..05e0c0cd6 100644 --- a/tests/test_comparercollection.py +++ b/tests/test_comparercollection.py @@ -451,6 +451,47 @@ def test_save_and_load_preserves_raw_model_data(cc, tmp_path): assert cc2["fake point obs"].raw_mod_data["m1"].name == "m1" +@pytest.fixture +def node_comparer() -> modelskill.comparison.Comparer: + """A comparer built by matching a NodeObservation against a NetworkModelResult (node gtype).""" + pytest.importorskip("networkx") + from modelskill.model.network import NetworkModelResult + from modelskill.network import Network, BasicNode, BasicReach + from modelskill.obs import NodeObservation + + time = pd.date_range("2019-01-01", periods=6, freq="D") + node_a_data = pd.DataFrame( + {"WaterLevel": [1.0, 2.0, 3.0, 4.0, 5.0, 6.0]}, index=time + ) + node_b_data = pd.DataFrame( + {"WaterLevel": [1.1, 2.2, 3.3, 4.4, 5.5, 6.6]}, index=time + ) + reach = BasicReach( + "r1", BasicNode("123", node_a_data), BasicNode("456", node_b_data), length=100.0 + ) + network = Network([reach]) + + nmr = NetworkModelResult(network, name="Network_Model") + node_id = network.find(node="123") + obs = NodeObservation(node_a_data, at=node_id, name="Node_123_Obs") + + return ms.match(obs, nmr) + + +def test_save_and_load_round_trips_node_gtype_raw_data(node_comparer, tmp_path): + """gh #677: node-gtype comparers must survive a save()/load() round trip like point comparers do.""" + cc = ms.ComparerCollection([node_comparer]) + fn = tmp_path / "test_cc_node.msk" + cc.save(fn) + + cc2 = ms.load(fn) + + assert cc2[0].gtype == "node" + assert len(cc2[0].raw_mod_data["Network_Model"]) == len( + node_comparer.raw_mod_data["Network_Model"] + ) + + # ======================== plotting ======================== From 9bc0e663d31fef61544f20d55f9132f6808919d7 Mon Sep 17 00:00:00 2001 From: jpalm3r Date: Fri, 24 Jul 2026 15:50:12 +0200 Subject: [PATCH 04/10] Flatten raw model data on save() for node gtype too Comparer.save() only flattened raw_mod_data into the netcdf for gtype == "point", so node comparers silently dropped their raw model data on save. Extend the check to also cover "node". Part of gh #677 --- src/modelskill/comparison/_comparison.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modelskill/comparison/_comparison.py b/src/modelskill/comparison/_comparison.py index aed4cc27e..5030e459f 100644 --- a/src/modelskill/comparison/_comparison.py +++ b/src/modelskill/comparison/_comparison.py @@ -1361,7 +1361,7 @@ def save(self, filename: Union[str, Path]) -> None: # https://docs.xarray.dev/en/stable/user-guide/io.html#groups # There is no need to save raw data for track data, since it is identical to the matched data - if self.gtype == "point": + if self.gtype in ("point", "node"): ds = self.data.copy() # copy needed to avoid modifying self.data for key, ts_mod in self.raw_mod_data.items(): From 6f1d37611e0c245c621036afc96ff427476b209d Mon Sep 17 00:00:00 2001 From: jpalm3r Date: Fri, 24 Jul 2026 15:52:43 +0200 Subject: [PATCH 05/10] Add node branch to Comparer.load() load() dispatched "track" / "vertical" / "point" and raised NotImplementedError for anything else, so a node comparer's netcdf could never be loaded back. Reconstruct NodeModelResult from the flattened _raw_* variables, mirroring the existing point branch, using the node coordinate carried along on each variable. Fixes gh #677 --- src/modelskill/comparison/_comparison.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/modelskill/comparison/_comparison.py b/src/modelskill/comparison/_comparison.py index 5030e459f..fcce0fcbf 100644 --- a/src/modelskill/comparison/_comparison.py +++ b/src/modelskill/comparison/_comparison.py @@ -1396,7 +1396,7 @@ def load(filename: Union[str, Path]) -> "Comparer": # FIXME: consider during Phase3 return Comparer(matched_data=data) - if data.gtype == "point": + if data.gtype in ("point", "node"): raw_mod_data: Dict[ str, PointModelResult @@ -1412,7 +1412,12 @@ def load(filename: Union[str, Path]) -> "Comparer": ds = data[[var_name]].rename( {"_time_raw_" + new_key: "time", var_name: new_key} ) - ts = PointModelResult(data=ds, name=new_key) + if data.gtype == "node": + ts = NodeModelResult( + data=ds, node=int(ds.coords["node"].item()), name=new_key + ) + else: + ts = PointModelResult(data=ds, name=new_key) raw_mod_data[new_key] = ts From 48b10134d46aa1183f2356e150f3bae03612e09c Mon Sep 17 00:00:00 2001 From: jpalm3r Date: Fri, 24 Jul 2026 15:53:36 +0200 Subject: [PATCH 06/10] Add failing test for unclear error on tz mismatch ms.match() raises a bare pandas TypeError ("Cannot compare tz-naive and tz-aware datetime-like objects") when the observation and model result disagree on timezone-awareness, with no ModelSkill-specific context about what went wrong. Refs gh #678 --- tests/test_match.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/test_match.py b/tests/test_match.py index 5664c341b..58fe78010 100644 --- a/tests/test_match.py +++ b/tests/test_match.py @@ -936,6 +936,26 @@ def test_match_obs_model_pos_args_wrong_order_helpful_error_message(): ms.match(mr, obs) +def test_match_raises_clear_error_on_tz_aware_vs_naive_mismatch(): + """gh #678: mismatched tz-awareness must raise a clear ModelSkill error, not a bare pandas TypeError.""" + mr = ms.PointModelResult( + data=pd.Series( + [1.0, 2.0, 3.0], index=pd.date_range("2020-01-01", periods=3, freq="h") + ), + name="Model", + ) + obs = ms.PointObservation( + data=pd.Series( + [1.0, 2.0, 3.0], + index=pd.date_range("2020-01-01", periods=3, freq="h", tz="UTC"), + ), + name="Station", + ) + + with pytest.raises(ValueError, match="[Tt]imezone"): + ms.match(obs, mr) + + def test_multiple_models_same_name(tmp_path: Path) -> None: obs = ms.PointObservation( "tests/testdata/SW/HKNA_Hm0.dfs0", item=0, x=4.2420, y=52.6887, name="HKNA" From 39c70f7db9e36f1d2e00acf47c7b6c2cefc30617 Mon Sep 17 00:00:00 2001 From: jpalm3r Date: Fri, 24 Jul 2026 15:59:27 +0200 Subject: [PATCH 07/10] Raise a clear error on tz-aware/tz-naive mismatch in match() No timezone handling existed anywhere in modelskill, so a mismatch between a tz-aware and tz-naive time index only surfaced as a bare pandas TypeError deep inside observation.trim(), with no indication of what went wrong. Validate tz-awareness compatibility between the observation and each model result before trimming, and raise a ValueError naming both sides. Fixes gh #678 --- src/modelskill/matching.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/modelskill/matching.py b/src/modelskill/matching.py index a514bf217..fe8ef4611 100644 --- a/src/modelskill/matching.py +++ b/src/modelskill/matching.py @@ -389,6 +389,24 @@ def _get_global_start_end(idxs: Iterable[pd.DatetimeIndex]) -> Period: return Period(start=min(starts), end=max(ends)) +def _check_timezone_compatibility( + observation: Observation, + raw_mod_data: Mapping[ + str, + PointModelResult | TrackModelResult | VerticalModelResult | NodeModelResult, + ], +) -> None: + obs_tz = observation.data.time.to_index().tz + for name, mr in raw_mod_data.items(): + mr_tz = mr.data.time.to_index().tz + if (obs_tz is None) != (mr_tz is None): + raise ValueError( + f"Timezone mismatch between observation '{observation.name}' " + f"(tz={obs_tz}) and model result '{name}' (tz={mr_tz}). " + "Both must be either timezone-aware or timezone-naive." + ) + + def _match_space_time( observation: Observation, raw_mod_data: Mapping[ @@ -402,6 +420,8 @@ def _match_space_time( idxs = [m.time for m in raw_mod_data.values()] period = _get_global_start_end(idxs) + _check_timezone_compatibility(observation, raw_mod_data) + observation = observation.trim(period.start, period.end, no_overlap=obs_no_overlap) if len(observation.data.time) == 0: return None From 7b52f8631d3d4522fd2c29bb70b1df00b862da65 Mon Sep 17 00:00:00 2001 From: jpalm3r Date: Fri, 24 Jul 2026 16:04:46 +0200 Subject: [PATCH 08/10] Annotate ts as PointModelResult | NodeModelResult in load() mypy inferred ts's type from its first assignment (NodeModelResult), flagging the later PointModelResult assignment as incompatible. Part of gh #677 --- src/modelskill/comparison/_comparison.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/modelskill/comparison/_comparison.py b/src/modelskill/comparison/_comparison.py index fcce0fcbf..8838184e5 100644 --- a/src/modelskill/comparison/_comparison.py +++ b/src/modelskill/comparison/_comparison.py @@ -1412,6 +1412,7 @@ def load(filename: Union[str, Path]) -> "Comparer": ds = data[[var_name]].rename( {"_time_raw_" + new_key: "time", var_name: new_key} ) + ts: PointModelResult | NodeModelResult if data.gtype == "node": ts = NodeModelResult( data=ds, node=int(ds.coords["node"].item()), name=new_key From d99b0ab49abc0a15dd59e6518a1e03ad3fa09aa7 Mon Sep 17 00:00:00 2001 From: jpalm3r Date: Fri, 24 Jul 2026 16:24:52 +0200 Subject: [PATCH 09/10] Remove explicit mention to issues --- tests/test_comparercollection.py | 2 +- tests/test_match.py | 2 +- tests/test_network.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_comparercollection.py b/tests/test_comparercollection.py index 05e0c0cd6..bdf4aa807 100644 --- a/tests/test_comparercollection.py +++ b/tests/test_comparercollection.py @@ -479,7 +479,7 @@ def node_comparer() -> modelskill.comparison.Comparer: def test_save_and_load_round_trips_node_gtype_raw_data(node_comparer, tmp_path): - """gh #677: node-gtype comparers must survive a save()/load() round trip like point comparers do.""" + """Node-gtype comparers must survive a save()/load() round trip like point comparers do.""" cc = ms.ComparerCollection([node_comparer]) fn = tmp_path / "test_cc_node.msk" cc.save(fn) diff --git a/tests/test_match.py b/tests/test_match.py index 58fe78010..3324f4788 100644 --- a/tests/test_match.py +++ b/tests/test_match.py @@ -937,7 +937,7 @@ def test_match_obs_model_pos_args_wrong_order_helpful_error_message(): def test_match_raises_clear_error_on_tz_aware_vs_naive_mismatch(): - """gh #678: mismatched tz-awareness must raise a clear ModelSkill error, not a bare pandas TypeError.""" + """Mismatched tz-awareness must raise a clear ModelSkill error, not a bare pandas TypeError.""" mr = ms.PointModelResult( data=pd.Series( [1.0, 2.0, 3.0], index=pd.date_range("2020-01-01", periods=3, freq="h") diff --git a/tests/test_network.py b/tests/test_network.py index ff734393a..6c2c4aa3c 100644 --- a/tests/test_network.py +++ b/tests/test_network.py @@ -593,7 +593,7 @@ def test_nodes_filtered_network_keeps_datetime_index(): A nodes-filtered network keeps the full topology, storing empty data for the unselected nodes. Concatenating those empty (RangeIndex) frames must not corrupt the DatetimeIndex, otherwise ms.match() later fails with - "time must be datetime". See gh #676. + "time must be datetime". """ path_to_file = "./tests/testdata/network.res1d" network = Network.from_res1d(path_to_file, nodes=["108", "101"], reaches=[]) From 85fc3017f1e88f6f78ae8d5c538c52afdc14740a Mon Sep 17 00:00:00 2001 From: jpalm3r Date: Fri, 24 Jul 2026 16:33:23 +0200 Subject: [PATCH 10/10] Moving check_timezone_compatibiltiy to flag timezone incompatibilities earlier Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/modelskill/matching.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/modelskill/matching.py b/src/modelskill/matching.py index fe8ef4611..b7ac4b983 100644 --- a/src/modelskill/matching.py +++ b/src/modelskill/matching.py @@ -417,11 +417,11 @@ def _match_space_time( spatial_tolerance: float, obs_no_overlap: Literal["ignore", "error", "warn"], ) -> xr.Dataset | None: + _check_timezone_compatibility(observation, raw_mod_data) + idxs = [m.time for m in raw_mod_data.values()] period = _get_global_start_end(idxs) - _check_timezone_compatibility(observation, raw_mod_data) - observation = observation.trim(period.start, period.end, no_overlap=obs_no_overlap) if len(observation.data.time) == 0: return None