Refresh DfsuModelResult against current mikeio - #707
Open
ecomodeller wants to merge 3 commits into
Open
ecomodeller wants to merge 3 commits into
ecomodeller wants to merge 3 commits into
Conversation
The dfsu adapter carried workarounds for mikeio behaviour that has since changed, held in place by a `mikeio >= 1.2` floor far below what the code actually targets. Raise the floor to 3.2 and drop what it was spanning. - Use the public `ds.z.elements` accessor (mikeio 3.2, DHI/mikeio#977) instead of `geometry.calc_ze(ds._zn)`, which reached into a private attribute. Rename `layer_boundaries` to `element_depths`: `calc_ze` returns z at element centers, as the assertion in test_vertical.py already assumed. - Read the column directly with `read(elements=elemids)` on Dfsu3D. The FIXME workaround read the full 3D field and selected afterwards; the direct read now returns identical values and z-coordinates. This also makes `elemids` load-bearing again - it was computed and checked for emptiness but otherwise unused. - Replace runtime `inspect.signature` introspection of `find_index` with a `n_layers` check. Only layered geometries accept a z argument, and this matches the idiom used a few lines above. Note that GeometryFMVerticalProfile is not a GeometryFM3D subclass but does take z, so an isinstance check against GeometryFM3D would lose z-indexing for it. The `np.sort(elemids)` guard stays: find_nearest_elements returns distance-ordered results, so it is doing real work rather than spanning old mikeio versions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Last read by the legacy Python 3.8 CI job, removed in bee6a58 (2024-10-23). Nothing in the tree has referenced it since, and it never listed mikeio, so it was not a second place to track the dependency floor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A dfsu file takes the Dfsu3D read(elements=...) branch; a Dataset takes sel(x, y). Only the latter had value-level assertions -- the two tests reaching the Dfsu3D branch check n_points and names, so a regression in that read would not fail CI. Assert the two branches agree on z and values, and that both match the dfsu column read directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
DfsuModelResultcarried workarounds for mikeio behaviour that has since changed, held in place by amikeio >= 1.2floor far below what the code actually targets. This raises the floor to 3.2 and drops what it was spanning.Changes
pyproject.toml—mikeio >= 1.2→mikeio >= 3.2. This is the enabling change;ds.zdoes not exist below 3.2. Note this is a breaking change for installs, not just a cleanup — worth a release note. Timing is good while we are on1.4.0a3.Public
zaccessor (dfsu.py:207).ds_column.geometry.calc_ze(ds_column._zn)→ds_column.z.elements._znis a private property (mikeio/dataset/_dataset.py:338); the public accessor landed in mikeio 3.2.0 (DHI/mikeio#977). Verified equivalent:np.allclose(ds.z.elements, ds.geometry.calc_ze(ds._zn))isTrue.Also renames
layer_boundaries→element_depths.calc_zedocuments its return as "Z-coordinates at element centers", not boundaries, and the assertion intest_vertical.pyalready called themelement_depths_expected.Drop the
Dfsu3Dread workaround (dfsu.py:196). The FIXME read the full 3D field and selected the column afterwards.read(elements=elemids, items=...)now works directly — onoresund_sigma_z.dfsuboth paths returnGeometryFMVerticalColumn (3, 5)with identical values and identicalcalc_ze. Side effect:elemidsis load-bearing again. It was computed at:181and checked for emptiness at:182-183, then never used.Replace
inspect.signatureintrospection (dfsu.py:261) withhasattr(self.data.geometry, "n_layers"), matching the idiom already used atdfsu.py:187-190.import inspectdrops out.An
isinstance(geometry, GeometryFM3D)check would have been wrong here:GeometryFMVerticalProfileis a sibling ofGeometryFM3Dunder_GeometryFMLayered, not a subclass, but itsfind_indexdoes acceptz. Then_layerscheck separates layered from 2D correctly —GeometryFM2D→False;GeometryFM3D,GeometryFMVerticalColumn,GeometryFMVerticalProfile→True— and stays on public API.Deliberately unchanged
np.sort(elemids)(dfsu.py:279-281) stays.find_nearest_elementsgenuinely returns distance-ordered results ([2850 2451 2851 2142 2485]onOresund2D.dfsu), so the guard is doing real work rather than spanning old mikeio versions, despite the comment.Longitude/Latitudevsx/ysniffing (dfsu.py:357-359) stays.extract_tracknames those columns by projection — a UTM-33 file returns['x', 'y', 'Surface elevation']— so the branch is still needed.Dfsu2DH/Dfsu3DvsDatasetbranching throughout.Dfsu3Dexposes onlyread,Dfsu2DHaddsextract_track, whileDatasethassel/isel/interp/extract_track. Closing that asymmetry is a mikeio-side change, out of scope here.Testing
737 passed, 60 skippedon the full suite. mypy clean ondfsu.py. The vertical path is covered bytests/model/test_vertical.pyagainsttests/testdata/oresund_sigma_z.dfsu; that test now asserts againstdfsu_col.z.elementsrather than the private attribute.🤖 Generated with Claude Code