Fix network save/load, tz-mismatch, and node-filter bugs found in 1.4.0 beta testing#681
Open
jpalm3r wants to merge 10 commits into
Open
Fix network save/load, tz-mismatch, and node-filter bugs found in 1.4.0 beta testing#681jpalm3r wants to merge 10 commits into
jpalm3r wants to merge 10 commits into
Conversation
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 <noreply@anthropic.com>
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
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
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
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
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
mypy inferred ts's type from its first assignment (NodeModelResult), flagging the later PointModelResult assignment as incompatible. Part of gh #677
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes three regressions found during 1.4.0 beta testing in the network matching and persistence workflow, improving robustness of node-filtered networks, node-geometry save/load, and timezone mismatch error reporting in ms.match().
Changes:
- Prevent
Network._build_dataframe()from concatenating empty node DataFrames (which degraded the time index toobjectdtype) by filtering out empty frames. - Add an explicit timezone-awareness compatibility check in
_match_space_time()that raises a clearValueErrorwhen obs/model timezones disagree. - Extend
Comparer.save()/Comparer.load()to supportgtype == "node"the same way as"point"for raw model data round-tripping, with regression tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_network.py | Adds regression coverage ensuring node-filtered networks preserve a DatetimeIndex time coordinate. |
| tests/test_match.py | Adds regression test asserting a clear ValueError on tz-aware vs tz-naive mismatch. |
| tests/test_comparercollection.py | Adds a node-geometry save/load round-trip regression test for raw model data. |
| src/modelskill/network.py | Filters out empty per-node frames before concat to avoid time index dtype corruption. |
| src/modelskill/matching.py | Introduces _check_timezone_compatibility() and calls it before trimming/alignment. |
| src/modelskill/comparison/_comparison.py | Enables node-geometry raw model data flattening on save and reconstruction on load. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…s earlier Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.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
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.
Summary
Three bugs found by a test user evaluating the network functionality ahead of the 1.4.0 release, each fixed test-first (failing regression test committed before the fix):
Network._build_dataframedegraded the time index toobjectdtype for nodes-filtered networks, because topology-only nodes store an emptyDataFrame(notNone), so theis not Nonefilter never dropped them beforepd.concat. Now also filters out empty frames.Comparer.save()/load()didn't support thenodegeometry type:save()only flattened raw model data forpoint, andload()raisedNotImplementedErrorfornode. Both now handlenodethe same way they already handlepoint.ms.match()raised a bare pandasTypeErrorwith no context when the observation and model result disagreed on timezone-awareness._match_space_timenow checks tz compatibility up front and raises a clearValueErrornaming both sides.Test plan
uv run ruff check srcuv run mypy src/ --config-file pyproject.tomluv run pytest --disable-warnings(full suite)uv run pytest src/modelskill/metrics.py --doctest-modules🤖 Generated with Claude Code