Add support for vertical mod cleaned by otzi5300 · Pull Request #609 · DHI/modelskill

otzi5300 · 2026-03-16T14:09:41Z

#close 596

Step 2 of 4 in implementation of vertical skills

General idea of vertical skill is to store data for model and obs in 1d (see below). Depth information is stored in auxilary coordinate z. Befinit of this compared to storing in 2d:

Similar to current data structures and current matching workflow can be used
Allowing varying z coordinates for obs and sigma layered models

Steps:

BRANCHED FROM Support vertical obs
Support vertical model items
Matching to get comparer
plotting/skill etc on vertical

Support vertical mod per commit

(see vertical skill notebook for more info)

Bugfix
Using a dataset to DfsuModelResults did not work. Sel_items and items was not set. Fixed.

VerticalModelResult class
Stores timeseries data for vertical profiles.

Support extraction of dfsu to VerticalModelResults

add _extract_vertical() in dfsu.py
Type-hints fro VerticlModelResults in comparer and spatial

Updated notebook vertical_skill.ipynb with examples for review process

Formatting
Reformatting TaylorPoint-line in comparer_plotter.py (Ruff wanted this)

Removing outcommented coded and fixing comments

Testdata

dfs0 file for creating VerticalModelResults

Tests
Testing various routes for creating model, edge cases and extracting VerticalModelResults from dfsu

Not sure why ruff want to change this line all of the sudden. To keep Ruff from complaining i will commit it.

* DfsuModelResults would fail if Dataset is passed as input since sel_items and item not set when extracting variables

ryan-kipawa

Looks great, super easy to follow! Looking forward to the upcoming PRs :)

ryan-kipawa · 2026-03-16T15:10:13Z

tests/model/test_vertical.py

+    # Test basic open for different formats
+    # ================
+    # dataframe input
+    def test_vertical_model_result_from_dataframe(self, vertical_model_df):


These three tests could be combined into one by parameterizing the input source (using pytest.mark.parameterize).

good point, fixed

ryan-kipawa · 2026-03-16T15:18:47Z

src/modelskill/model/vertical.py

+    keep_duplicates : (str, bool), optional
+        Strategy for handling duplicate timestamps (wraps xarray.Dataset.drop_duplicates)
+        "first" to keep first occurrence, "last" to keep last occurrence,
+        False to drop all duplicates, "offset" to add milliseconds to


Does it make sense to use a string instead of False, so that they're all string literals with nice completion?

that is a good idea but other observations and models (point, track) uses "first" "last" false.

my suggestion is to keep the implementation of vertical as it is but open an separate issue to deal with this for all types of observations and models. Also this type of argument is consistent with pandas https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.duplicated.html

ryan-kipawa · 2026-03-16T15:24:05Z

tests/model/test_vertical.py

+
+        assert mr.equals(mr2)
+        assert mr2.gtype == mr.gtype
+        assert mr2.name == mr.name


If you update one of these on mr, does mr2 remain in tact or is it also changed? I've encountered something like this before in modelskill, and it's not always obvious that it's happening, and caused some extra effort to sort out

i have added test that changing the name in original does not alter name in the roundtripped

ryan-kipawa · 2026-03-16T15:33:41Z

tests/model/test_vertical.py

+        ntimes = len(np.unique(vmr.data.time.values))
+        n_layers = int(len(vmr.data.z.values) / ntimes)
+
+        assert n_layers == n_layers_expected


This asserts correct shape. Should we also have an assertion that the correct values are being extracted? Maybe not all of them, but first and last.

ryan-kipawa · 2026-03-16T15:36:19Z

src/modelskill/model/vertical.py

@@ -0,0 +1,77 @@
+from __future__ import annotations
+from typing import Any, Literal, Optional, Sequence


Optional is old school typing. It's preferred to use type | None.

Changed but now it is not fully consistent with the rest of the code. Maybe we should consider changing the rest of the code as well

ecomodeller

mikeio.read -> Dfsu3d.read already supports reading a column with x,y, see this notebook
https://github.com/DHI/mikeio/blob/main/notebooks/Dfsu%20-%203D%20sigma-z.ipynb

ecomodeller · 2026-03-17T08:29:03Z

src/modelskill/model/dfsu.py

+            elif isinstance(self.data, mikeio.dfsu.Dfsu3D):
+                # FIXME: open with specific element instead...make sure mikeio fix is in place before
+                # ds_column = self.data.read(elements=elemids) # when bug is fixed in mikeio
+                ds_column = self.data.read(items=self.sel_items.all).sel(x=x, y=y)


Why not self.data.read(..., x=x,y=y)? Skip the .sel

ecomodeller · 2026-03-17T08:29:51Z

src/modelskill/model/dfsu.py

+
+        if not self._in_domain(x, y):
+            raise ValueError(
+                f"PointObservation '{observation.name}' (x={x}, y={y}) outside model domain!"


ecomodeller · 2026-03-17T08:32:00Z

src/modelskill/matching.py

                aligned = pmr.align(observation, max_gap=max_model_gap)
+            case VerticalModelResult(), VerticalObservation():
+                raise NotImplementedError("Vertical matching not implemented yet!")
+                # aligned = vmr.align(observation, max_gap=max_model_gap)


Add TODO to make this findable?

ecomodeller · 2026-03-17T08:33:13Z

src/modelskill/model/dfsu.py

+        time_flat = np.repeat(ds_column.time, n_layers)
+        z_flat = layer_boundaries.flatten()  # Flatten z-coordinates
+        item_values_1d = ds_column[item_name].to_numpy().flatten()  # Flatten item?
+        # aux_items_flat = {aux_item: ds_column[aux_item].to_numpy().flatten() for aux_item in self.sel_items.aux}


Should the commented code stay or be deleted?

ecomodeller · 2026-03-17T09:01:29Z

src/modelskill/model/dfsu.py

+                f"PointObservation '{observation.name}' (x={x}, y={y}) outside model domain!"
+            )
+
+        elemids = self.data.geometry.find_index(x=x, y=y)


Not necessary

otzi5300 added 6 commits March 16, 2026 09:29

formatting

73a354a

Not sure why ruff want to change this line all of the sudden. To keep Ruff from complaining i will commit it.

remove unused code & correct comment

5437d3d

Add VerticalModelResult class, imports and tests

0e4ef27

sel_items and item must be set for a dataset

9b240f3

* DfsuModelResults would fail if Dataset is passed as input since sel_items and item not set when extracting variables

support extract from dfsu and test

5c9b35c

update notebook dev examples

dbcecb3

otzi5300 linked an issue Mar 16, 2026 that may be closed by this pull request

Add ModelResult for vertical column #596

Closed

otzi5300 requested review from ecomodeller and ryan-kipawa March 16, 2026 14:12

otzi5300 mentioned this pull request Mar 16, 2026

Add support for vertical mod #606

Closed

ryan-kipawa approved these changes Mar 16, 2026

View reviewed changes

otzi5300 added 3 commits March 17, 2026 08:52

use parametrize for test

be5012c

extra check for roundtrip test

9d7bafc

also check verticalModel depth values

c585d67

otzi5300 removed the request for review from ecomodeller March 17, 2026 09:06

Optional to type | none

6b214d4

otzi5300 merged commit ea4c9e3 into vertical-skill Mar 17, 2026

ecomodeller reviewed Mar 17, 2026

View reviewed changes

otzi5300 deleted the add_support_for_vertical_mod_cleaned branch March 17, 2026 09:13

otzi5300 linked an issue Mar 17, 2026 that may be closed by this pull request

Matching VerticalObservation and ColumnModelResults #603

Open

		@@ -0,0 +1,77 @@
		from __future__ import annotations
		from typing import Any, Literal, Optional, Sequence

Conversation

otzi5300 commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Step 2 of 4 in implementation of vertical skills

Support vertical mod per commit

Uh oh!

ryan-kipawa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

otzi5300 Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

otzi5300 Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecomodeller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

otzi5300 commented Mar 16, 2026 •

edited

Loading

otzi5300 Mar 17, 2026 •

edited

Loading

otzi5300 Mar 17, 2026 •

edited

Loading