refactor: drop clustering dim-rename layer via tsam-xarray DimNames#739
Conversation
|
Warning Review limit reached
Next review available in: 41 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 (4)
📝 WalkthroughWalkthroughThe clustering pipeline removes reserved-dimension rename/unrename handling, configures tsam_xarray with ChangesClustering dimension handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
63c717f to
55923b3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
flixopt/transform_accessor.py (1)
1424-1437: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
dim_namesisn't guarded byreserved_tsam_keys.
dim_names=tsam_xarray.DimNames(period='original_cluster')is now passed explicitly totsam_xarray.aggregate()alongside**tsam_kwargs_full. Since**tsam_kwargsis documented as forwarded "for forward compatibility" totsam.aggregate(), a caller passingdim_names=...viacluster(**tsam_kwargs)will hit an unguarded PythonTypeError("multiple values for keyword argument") instead of the friendlyValueErrorthatreserved_tsam_keysproduces for the other explicit parameters (Lines 1325-1337).🛡️ Proposed fix
reserved_tsam_keys = { 'n_clusters', 'period_duration', # exposed as cluster_duration 'temporal_resolution', # computed automatically 'timestep_duration', # computed automatically 'cluster', 'segments', 'extremes', 'preserve_column_means', 'rescale_exclude_columns', 'round_decimals', 'numerical_tolerance', + 'dim_names', # fixed internally to avoid original-period/slice-dim collisions }🤖 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/transform_accessor.py` around lines 1424 - 1437, Guard the explicit dim_names argument in the clustering flow before calling tsam_xarray.aggregate, alongside the other reserved parameters checked by reserved_tsam_keys. Ensure a caller-provided dim_names in tsam_kwargs is rejected with the existing friendly ValueError rather than causing duplicate-keyword TypeError, while preserving the explicit original_cluster value for valid calls.flixopt/clustering/base.py (1)
292-309: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale docstring:
aggregation_resultstill describes the removed rename adapter.This docstring says flixopt's reserved-dim renames are still applied to the raw result (citing an internal
_perioddim) and thatoriginal/reconstructed/residuals/accuracyoffer a "friendlier" alternative. After this refactor those properties (Lines 311-356) return_aggregation_result's values directly with no unrename step, so they now expose identical dims toaggregation_resultitself — the described distinction no longer holds and will confuse users choosing between the two.📝 Suggested docstring update
- The returned object holds the **raw** tsam_xarray result, on which - flixopt's reserved-dim renames are still applied (the period dim is - ``_period``). For a friendlier view with the original dim names, use - the ``original`` / ``reconstructed`` / ``residuals`` / ``accuracy`` - properties or ``compare()`` instead. + The returned object holds the tsam_xarray result directly (no + dim renaming is applied). For convenience accessors that align + ``reconstructed`` to ``original``'s dim order, or for a tidy + original-vs-clustered Dataset, use ``original`` / ``reconstructed`` / + ``residuals`` / ``accuracy`` / ``compare()`` instead.🤖 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/clustering/base.py` around lines 292 - 309, Update the aggregation_result docstring to remove references to the removed rename adapter, the internal _period dimension, and the claim that original/reconstructed/residuals/accuracy provide a friendlier view. Document that these properties expose the same underlying aggregation-result dimensions and values directly, while preserving the availability and ValueError guidance.
🧹 Nitpick comments (1)
flixopt/transform_accessor.py (1)
1357-1372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate slice-dim
expand_dimslogic vsapply_clustering().This period/scenario
expand_dimsloop (1362-1372) is essentially the same block repeated inapply_clustering()(Lines 1504-1509), just iterating a different dim list. Worth extracting a small helper (e.g._ensure_slice_dims(da, dim_names, ds)) shared by both call sites to avoid future drift.🤖 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/transform_accessor.py` around lines 1357 - 1372, Extract the repeated period/scenario dimension normalization into a shared helper such as _ensure_slice_dims(da, dim_names, ds), preserving the existing coordinate-dropping and expand_dims behavior. Replace the inline loop near da_for_clustering and the corresponding logic in apply_clustering() with calls to this helper, passing each caller’s appropriate dimension list.
🤖 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.
Outside diff comments:
In `@flixopt/clustering/base.py`:
- Around line 292-309: Update the aggregation_result docstring to remove
references to the removed rename adapter, the internal _period dimension, and
the claim that original/reconstructed/residuals/accuracy provide a friendlier
view. Document that these properties expose the same underlying
aggregation-result dimensions and values directly, while preserving the
availability and ValueError guidance.
In `@flixopt/transform_accessor.py`:
- Around line 1424-1437: Guard the explicit dim_names argument in the clustering
flow before calling tsam_xarray.aggregate, alongside the other reserved
parameters checked by reserved_tsam_keys. Ensure a caller-provided dim_names in
tsam_kwargs is rejected with the existing friendly ValueError rather than
causing duplicate-keyword TypeError, while preserving the explicit
original_cluster value for valid calls.
---
Nitpick comments:
In `@flixopt/transform_accessor.py`:
- Around line 1357-1372: Extract the repeated period/scenario dimension
normalization into a shared helper such as _ensure_slice_dims(da, dim_names,
ds), preserving the existing coordinate-dropping and expand_dims behavior.
Replace the inline loop near da_for_clustering and the corresponding logic in
apply_clustering() with calls to this helper, passing each caller’s appropriate
dimension list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c57d10a-a8bd-406f-9e87-2b5be79abf07
📒 Files selected for processing (4)
flixopt/clustering/base.pyflixopt/transform_accessor.pypyproject.tomltests/test_clustering/test_base.py
tsam-xarray 0.6.4 makes output dimension names configurable through DimNames. Pass DimNames(period='original_cluster') to aggregate() so the original-period axis no longer collides with a real 'period' slice dim. This removes the entire rename/unrename adapter that existed only to work around the fixed 'period'/'cluster' output names: - cluster()/apply_clustering() no longer rename period->_period etc. in and back out, and apply_clustering() applies the stored ClusteringResult directly instead of rebuilding one with renamed slice dims. - _ReducedFlowSystemBuilder and Clustering lose _unrename_map/_unrename. The only renames left are the honest tsam<->flixopt boundary translations timestep->time / timestep->segment. Bumps the tsam_xarray pin to 0.6.4. All public clustering outputs (cluster_assignments, cluster_occurrences, original/reconstructed/residuals/accuracy, dim_names, disaggregate, the reduced FlowSystem's (cluster, time) vars and cluster_weight) keep identical dim names, shapes and values in single- and multi-period systems. BREAKING CHANGE: the serialized clustering format changed - slice dims are now stored as 'period' (was '_period') plus a new 'dim_names' key. Clustering artifacts saved to JSON/netCDF by an older flixopt no longer load for apply_clustering()/expand() in this version; re-run transform.cluster() to regenerate them. Fresh clustering and same-version round-trips are unaffected. Also raises the minimum tsam_xarray to 0.6.4. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
55923b3 to
c4a1915
Compare
1 participant
Context
flixopt's clustering already delegates all aggregation math to
tsam_xarray. The only meaningful incidental complexity left was a dim-name adapter layer:tsam_xarrayhard-coded its output dim names (cluster,timestep,period,segment), and theperiodoutput axis collided with flixopt's ownperiodslice dim in multi-period systems. To dodge that,cluster()renamedperiod → _period/cluster → _clusteron the way in and unrenamed on the way out, with an_unrename_mapthreaded throughcluster(),apply_clustering(),_ReducedFlowSystemBuilder, andClustering.tsam_xarray#99 → tsam-xarray 0.6.4 shipped a
DimNamesconfig that makes those output names configurable.Change
Pass
dim_names=DimNames(period='original_cluster')toaggregate(), so the original-period axis is namedoriginal_clusternatively and never collides with aperiodslice dim. That lets the whole rename/unrename layer go:cluster()/apply_clustering()no longer rename dims in and back out.apply_clustering()now applies the storedClusteringResultdirectly instead of rebuilding one with renamedslice_dims._ReducedFlowSystemBuilderandClusteringlose_unrename_map/_unrename.timestep→time/timestep→segment(tsam's integer sub-index vs flixopt's datetime axis; the new upstream collision guard blocks collapsing these anyway).tsam_xarraypin0.6.1 → 0.6.4.test_basemock helper now buildsClusteringResults with the sameDimNames(period='original_cluster')convention production uses.Net −83 lines, entirely subtraction of adapter code.
The serialized clustering format changed: slice dims are now stored as
period(was_period), plus a newdim_nameskey.transform.cluster()and same-version round-trips are unaffected (verified:to_dataset/from_dataset,to_netcdf,to_json).apply_clustering()/expand()in this version — the stored legacy_periodslice dim no longer matches the liveperioddim.Migration: re-run
transform.cluster()to regenerate any persisted clustering artifacts. Also raises the minimumtsam_xarrayto0.6.4.No runtime API change
Every public clustering output keeps identical dim names, shapes, and values in single- and multi-period systems:
cluster_assignments(original_cluster[, period]),cluster_occurrences,original/reconstructed/residuals/accuracy,dim_names,disaggregate(),compare(), and the reduced FlowSystem's(cluster, time)vars +cluster_weight. The old code reached those names via internal renames; tsam now emits them directly.Verification
tests/test_clustering/(IO/serialization, integration, multiperiod extremes, segmentation, reduce/expand, expansion-regression) +tests/test_math/test_clustering.py, on HiGHS and Gurobi.ruff check+ruff formatclean; pre-commit hooks pass.🤖 Generated with Claude Code