feat(cluster): original-vs-clustered comparison accessors (fixes #728)#734
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughChangesClustering comparison API
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Clustering
participant AggregationResult
participant xrDataset
User->>Clustering: call compare(variable)
Clustering->>AggregationResult: retrieve full aggregation data
AggregationResult-->>Clustering: return original and reconstructed arrays
Clustering->>xrDataset: build original and clustered variables
xrDataset-->>User: return comparison dataset
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@docs/user-guide/migration-guide-v7.md`:
- Around line 93-96: The migration guide should describe
fs_clustered.clustering.plot.compare() as deprecated but still temporarily
supported with a deprecation warning, and recommend clustering.compare() for new
code. In docs/user-guide/optimization/clustering.md lines 154-156, replace
“removed” with “deprecated” and explicitly mention backward compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f95e344a-1fcc-4b06-b572-baf5ef3518ac
📒 Files selected for processing (5)
docs/user-guide/migration-guide-v7.mddocs/user-guide/optimization/clustering.mdflixopt/clustering/base.pyflixopt/plot_result.pytests/test_clustering/test_integration.py
c9707d4 to
f797738
Compare
f797738 to
0ccc76d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@docs/notebooks/08c-clustering.ipynb`:
- Around line 149-161: Update the notebook snippet import to include
ExtremeConfig alongside ClusterConfig, so the existing ExtremeConfig usage in
the clustering example is defined when copied and executed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44bb2bdc-81ee-473e-baeb-e2d668508ae3
📒 Files selected for processing (6)
docs/notebooks/08c-clustering.ipynbdocs/user-guide/migration-guide-v7.mddocs/user-guide/optimization/clustering.mdflixopt/clustering/base.pyflixopt/plot_result.pytests/test_clustering/test_integration.py
🚧 Files skipped from review as they are similar to previous changes (4)
- flixopt/plot_result.py
- docs/user-guide/optimization/clustering.md
- tests/test_clustering/test_integration.py
- flixopt/clustering/base.py
81e6b11 to
b9f3fa5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@docs/user-guide/migration-guide-v7.md`:
- Around line 142-143: Update the migration guide example around
clustering.compare(var) to define var before it is used, or replace it with a
representative variable-name string consistent with the earlier example, so the
snippet runs without a NameError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2516c709-7ddb-44d0-a990-844200016a5c
📒 Files selected for processing (6)
docs/notebooks/08c-clustering.ipynbdocs/user-guide/migration-guide-v7.mddocs/user-guide/optimization/clustering.mdflixopt/clustering/base.pyflixopt/plot_result.pytests/test_clustering/test_integration.py
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/user-guide/optimization/clustering.md
- flixopt/plot_result.py
- docs/notebooks/08c-clustering.ipynb
b9f3fa5 to
2efb3bd
Compare
Restore a first-class way to compare original vs clustered profiles, which regressed when clustering.plot.compare() was removed in v7 (#655). Reported in discussion #728. New accessors on Clustering (pre-serialization): - compare(variable=None) -> tidy Dataset(original, clustered), plot-ready - original / reconstructed / residuals / accuracy These un-rename tsam_xarray's internal dims (_period -> period) and align original/reconstructed dim order, so users select with the natural coord names instead of the raw aggregation_result's _period. Plotting is left to the user: compare() returns tidy data, so a chart is a one-liner (px.line(compare(var).to_dataframe()[['original','clustered']])). The tidy data helper is now upstream in tsam_xarray (FBumann/tsam_xarray#92). Docs: - migration-guide-v7: compare recipe (timeseries + duration curve) and multi-variable/period/scenario notes - clustering.md: replace stale clustering.metrics section (removed in v7) with clustering.accuracy / compare() - 08c-clustering notebook: demo cell plotting compare() with plotly express - fix broken tsam_xarray repo links (FZJ-IEK3-VSA -> FBumann) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2efb3bd to
1d4eb31
Compare
1 participant
Motivation
Discussion #728 asks how to compare original vs clustered profiles now that
fs_clustered.clustering.plot.compare()was removed in v7 (#655). The v7 migration guide pointed ataggregation_result, but using it directly exposes a papercut: flixopt renamesperiod→_periodbefore calling tsam_xarray, andaggregation_resultreturns that raw result, so users must.sel(_period=...).Changes
New convenience accessors on
Clustering(pre-serialization):compare(variable=None)→ tidyxr.Dataset(original, clustered)on the original time axis, plot-ready — no reshaping.original/reconstructed/residuals/accuracy.These un-rename the internal dims (
_period→period) and alignoriginal/reconstructeddim order, so selection uses natural coord names. Works across variables × periods × scenarios.Plotting is left to the user.
compare()returns tidy data, so a chart is a one-liner:No
plot.compare()method is added — it would be maintained plotting surface (styling,kind=, plotly dep) duplicating what one line of the user's own library does. The tidy-data helper this builds on is now upstream in tsam_xarray (#92, completed).Docs:
migration-guide-v7.md— compare recipe (timeseries + duration curve) + multi-variable/period/scenario notes.clustering.md— replaced the stale "Clustering Quality Metrics" section (documented the removedclustering.metrics) withclustering.accuracy/compare().08c-clusteringnotebook — demo cell plottingcompare()with plotly express.tsam_xarrayrepo links (FZJ-IEK3-VSA→FBumann).Tests
TestClusteringCompare: dim alignment, residuals identity, tidycompare(), plot-ready columns, accuracy exposure,period(not_period) naming, pre-serialization guards. All green; ruff clean;mkdocs build --strictpasses.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
compare()to generate tidy original-versus-clustered datasets, with optional variable selection.Documentation
compare()workflow.Tests
compare(), and serialization-gated behavior.