Skip to content

Fix network save/load, tz-mismatch, and node-filter bugs found in 1.4.0 beta testing - #681

Open
jpalm3r wants to merge 3 commits into
mainfrom
beta_test_found_bugs
Open

Fix network save/load, tz-mismatch, and node-filter bugs found in 1.4.0 beta testing#681
jpalm3r wants to merge 3 commits into
mainfrom
beta_test_found_bugs

Conversation

@jpalm3r

@jpalm3r jpalm3r commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

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):

Test plan

  • uv run ruff check src
  • uv run mypy src/ --config-file pyproject.toml
  • uv run pytest --disable-warnings (full suite)
  • uv run pytest src/modelskill/metrics.py --doctest-modules
  • Each fix has a dedicated regression test that failed before the fix and passes after

🤖 Generated with Claude Code

@jpalm3r
jpalm3r requested a review from ecomodeller as a code owner July 24, 2026 14:10
Copilot AI lite review requested due to automatic review settings July 24, 2026 14:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to object dtype) by filtering out empty frames.
  • Add an explicit timezone-awareness compatibility check in _match_space_time() that raises a clear ValueError when obs/model timezones disagree.
  • Extend Comparer.save()/Comparer.load() to support gtype == "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.

Copilot AI review requested due to automatic review settings July 24, 2026 14:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comment thread src/modelskill/matching.py Outdated
Copilot AI review requested due to automatic review settings July 24, 2026 14:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 23 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 23 changed files in this pull request and generated no new comments.

Suppressed comments (2)

tests/test_network.py:624

  • This test name and body no longer align: it says "from_res1d" and "empty_nodes_and_reaches", but it calls Network.from_mike() with non-empty nodes and duplicates the datetime-index assertions already covered by test_nodes_filtered_network_keeps_datetime_index(). Consider removing this redundant test (empty nodes/reaches is already covered below by test_from_mike_empty_nodes_and_reaches_keeps_topology_and_empty_outputs).
def test_from_res1d_empty_nodes_and_reaches_keeps_topology_and_empty_outputs():
    path_to_file = "./tests/testdata/network.res1d"
    network = Network.from_mike(path_to_file, nodes=["108", "101"], reaches=[])

    assert isinstance(network._df.index, pd.DatetimeIndex)

src/modelskill/network.py:425

  • The PR description focuses on three bug fixes (#676#678), but this change also introduces a public API shift: Network.from_res1d is removed and replaced by Network.from_mike plus a new Network.from_epanet (with companion file handling). Please either (a) update the PR description to explicitly call out this API change and its migration path, or (b) consider adding a compatibility shim (e.g., from_res1d = from_mike) if any released version could have external callers depending on the old name.
    def from_mike(
        cls,
        res: str | Path | Res1D,
        *,
        nodes: str | list[str] | None = None,
        reaches: str | list[str] | None = None,
    ) -> Network:

jpalm3r and others added 3 commits August 11, 2026 10:08
Matching a tz-aware observation against a tz-naive model result (or the
reverse) failed deep inside pandas with a bare TypeError. Check awareness
up front and name both sides in the message.

Fixes gh #678.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Raw model data was only round-tripped for the "point" gtype, so a node
Comparer came back without it and the reconstructed series were plain
PointModelResults. Handle "node" alongside "point" and rebuild each
series as a NodeModelResult.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A node carrying an empty frame still contributed a column block, which
gave the concatenated result a non-datetime index. Treat empty data the
same as missing data.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jpalm3r
jpalm3r force-pushed the beta_test_found_bugs branch from e6e2002 to 2d017ee Compare August 11, 2026 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants