Skip to content

fix: load clustered NetCDF files written before flixopt 7.0 - #757

Merged
FBumann merged 2 commits into
mainfrom
fix/pre-v7-clustering-load
Jul 27, 2026
Merged

fix: load clustered NetCDF files written before flixopt 7.0#757
FBumann merged 2 commits into
mainfrom
fix/pre-v7-clustering-load

Conversation

@FBumann

@FBumann FBumann commented Jul 27, 2026

Copy link
Copy Markdown
Member

Problem

Any clustered NetCDF file written before flixopt 7.0 fails to load on main:

OSError: Failed to load FlowSystem from NetCDF file ...:
Failed to create instance of Clustering:
Clustering.__init__() got unexpected keyword arguments: {'results'}

#742 fixed the dangling _original_data_refs / _metrics_refs in these files, but the wrapper schema itself also changed. Pre-7.0 files store:

{"results": {"dim_names": ["period"], "results": {"2030": {...tsam blob...}}}}

while Clustering now expects a clustering_result matching tsam_xarray.ClusteringResult.from_dict:

{"clustering_result": {"time_dim": "time", "slice_dims": ["_period"],
                       "clusterings": [{"key": [2030], "clustering": {...tsam blob...}}]}}

The per-slice tsam blobs are byte-identical between the two, so only the surrounding structure needs rewriting.

Two details that matter

Key types. Legacy keys are strings ('2030', '2030|low'), but the current schema indexes clusterings by the coordinate values themselves — '2030' must become the integer 2030 or every lookup misses and the assignments silently collapse onto one slice. The value is recovered by matching against the restored coordinate rather than by guessing a type, so a scenario label that merely looks numeric survives unchanged.

Slice dim spelling. period is handed to tsam_xarray as _period today, and Clustering infers its un-rename map from exactly that, so legacy dim_names are translated rather than copied.

Verification

Generated real fixtures from flixopt 6.1.0 and 6.2.1 in isolated environments — plain, solved, multi-period, and period+scenario. All load and expand:

fixture load expand assignments
6.2.1 plain ok 720 timesteps preserved
6.2.1 solved ok (solution restored) 720 timesteps preserved
6.2.1 multi-period ok 480 timesteps preserved per period
6.2.1 period+scenario ok 480 timesteps preserved per (period, scenario)
6.1.0 plain ok 720 timesteps preserved

Cluster assignments compare equal against the values read straight out of the legacy JSON, per slice. For the solved file, every one of the 30 original clusters in the expanded solution reproduces its typical cluster's flow_rate exactly.

tests/test_clustering: 291 passed (286 before, 5 added).

Note on tests

The new tests rewrite a current dataset into the legacy layout rather than committing a binary fixture — the layout was verified byte-for-byte against the real 6.x files above. system_with_periods_and_scenarios moves to module scope so they can reuse its deliberately-different per-slice assignments, which is what makes a mis-keyed slice detectable.

Note on #755

Local runs use tsam_xarray==0.6.1 to match main's pin. With 0.6.5 installed, tests/test_clustering fails for the unrelated cluster_weights deprecation that #755 fixes — independent of this change.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Added compatibility for loading datasets created with pre-7.0 clustering formats.
    • Preserved cluster assignments and weights during migration, including multidimensional period and scenario slices.
    • Ensured legacy clustered datasets continue to support loading, NetCDF roundtrips, and timestep expansion.

Release

The pending release PR proposes 8.0.0, but its only breaking change comes from #739, which was reverted in #746 — release-please does not pair a revert with its original, so the BREAKING CHANGE: footer still counts. The behaviour it describes is not in the code.

Verified against artifacts from released versions: files written by 7.2.3 (plain and multi-period) load and expand on current main unchanged, and with this PR 6.1.0 and 6.2.1 files load too. The release is strictly more compatible than 7.2.1, not less.

Hence the footer below, so release-please cuts a minor instead. The ⚠ BREAKING CHANGES section it generates from #739 is now inaccurate and should be dropped from CHANGELOG.md in the release PR before merging.

Release-As: 7.3.0

Files written before 7.0 serialize the clustering under a `results` key
holding {'dim_names': [...], 'results': {key: tsam_blob}}, where each key
is the slice coordinates joined into a string ('2030|low', or
'__single__' when the clustering is undivided). Clustering now expects a
`clustering_result` matching tsam_xarray's ClusteringResult.from_dict,
which takes a list of {'key': [...], 'clustering': tsam_blob} entries, so
loading any pre-7.0 clustered file fails with:

    Failed to create instance of Clustering:
    Clustering.__init__() got unexpected keyword arguments: {'results'}

The per-slice tsam blobs are byte-identical between the two layouts, so
only the surrounding structure is rewritten. Two details matter:

- Legacy keys are strings, but the current schema indexes clusterings by
  the coordinate values, so '2030' has to become the integer 2030 or
  every lookup misses. The value is recovered by matching against the
  restored coordinate rather than by guessing a type, so scenario labels
  that merely look numeric survive unchanged.
- Slice dims are stored under their pre-rename spelling, so 'period'
  becomes '_period' to match what tsam_xarray is handed today.

Verified against files generated by flixopt 6.1.0 and 6.2.1 — plain,
solved, multi-period, and period+scenario. Cluster assignments compare
equal per slice, and an expanded solved file reproduces each original
cluster from its typical cluster.

The multi-dim fixture moves to module scope so the new tests can reuse
its deliberately-different per-slice assignments, which is what catches
keys landing on the wrong slice.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Pre-7.0 serialized clustering data is migrated into the current schema during restoration. Legacy slice keys are converted using restored FlowSystem indices, with compatibility tests covering assignments, NetCDF roundtrips, expansion, and multidimensional slices.

Changes

Legacy clustering migration

Layer / File(s) Summary
Restore and migrate legacy clustering
flixopt/io.py
Legacy results structures are converted into clustering_result; stringified slice keys are mapped to restored coordinates before clustering and weights are resolved.
Validate legacy clustering compatibility
tests/test_clustering/test_clustering_io.py
Tests verify migration behavior, assignment preservation, NetCDF roundtrips, timestep expansion, and multidimensional period/scenario key mapping.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • flixOpt/flixopt#549: Introduced the newer clustering serialization and restoration structures handled by this migration.
  • flixOpt/flixOpt#654: Also addresses legacy results payload handling during clustering deserialization.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely states the main change: loading clustered NetCDF files written before flixopt 7.0.
Description check ✅ Passed The description is detailed and covers the bug, legacy schema rewrite, key handling, slice-dim translation, and verification results.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/pre-v7-clustering-load

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

🧹 Nitpick comments (1)
flixopt/io.py (1)

1925-1935: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Silent mismatches make legacy load failures hard to diagnose.

Two silent paths: zip(..., strict=False) drops extra parts when a label contains |, and an unmatched coordinate falls back to the raw string, which will never match a slice downstream. A log line at each point keeps behavior identical but makes the eventual failure traceable.

🧭 Proposed logging
     parts = key.split(FlowSystemDatasetIO.LEGACY_KEY_SEPARATOR) if len(legacy_dims) > 1 else [key]
+    if len(parts) != len(legacy_dims):
+        logger.warning(
+            f'Legacy clustering key {key!r} splits into {len(parts)} parts for dims {legacy_dims}; '
+            'slice mapping may be incorrect.'
+        )
 
     restored: list[Any] = []
     for dim, part in zip(legacy_dims, parts, strict=False):
         index = getattr(flow_system, f'{dim}s', None)
         match = next((value for value in index if str(value) == part), None) if index is not None else None
         if match is None:
+            logger.warning(f'No {dim} coordinate matches legacy clustering key part {part!r}; keeping raw string.')
             restored.append(part)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@flixopt/io.py` around lines 1925 - 1935, Update the legacy key restoration
loop around FlowSystemDatasetIO.LEGACY_KEY_SEPARATOR to log when split parts and
legacy_dims differ before zip truncates values, and log when a coordinate lookup
has no match before appending the raw part. Keep the existing restoration
behavior unchanged, including numpy scalar conversion and raw-string fallback,
while making both silent mismatch paths traceable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@flixopt/io.py`:
- Around line 1925-1935: Update the legacy key restoration loop around
FlowSystemDatasetIO.LEGACY_KEY_SEPARATOR to log when split parts and legacy_dims
differ before zip truncates values, and log when a coordinate lookup has no
match before appending the raw part. Keep the existing restoration behavior
unchanged, including numpy scalar conversion and raw-string fallback, while
making both silent mismatch paths traceable.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 52268823-6395-4399-bab7-eb8dd6eb5da7

📥 Commits

Reviewing files that changed from the base of the PR and between f280bda and b393d87.

📒 Files selected for processing (2)
  • flixopt/io.py
  • tests/test_clustering/test_clustering_io.py

The key restoration was a second method scanning the coordinate index once
per key part. Building a stringified lookup per slice dim up front collapses
it into the migration itself, at the same cost.

Behaviour is unchanged: 291 clustering tests pass, and files generated by
flixopt 6.1.0 and 6.2.1 (plain, solved, multi-period, period+scenario) still
load with assignments equal per slice.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@FBumann
FBumann enabled auto-merge (squash) July 27, 2026 15:07
@FBumann
FBumann merged commit f8e53bd into main Jul 27, 2026
11 checks passed
FBumann added a commit that referenced this pull request Jul 27, 2026
Retarget the pending release from 8.0.0 to 7.3.0.

The major came solely from the BREAKING CHANGE footer on #739, which was
reverted in #746. Release-please does not pair a revert with its original,
so the footer still counted. The behaviour it describes is not in the
released code: files written by 7.2.3 load and expand unchanged on main,
and since #757 files written by 6.1.0 and 6.2.1 load as well.

The breaking notice is dropped and #739 is listed under Reverts instead of
Code Refactoring, since it nets to zero in this release.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant