feat(cluster): add cluster_on for true subset clustering#724
Open
FBumann wants to merge 3 commits into
Open
Conversation
Reintroduce the ergonomics of the removed v5/v6 `data_vars` parameter as
`transform.cluster(cluster_on=[...])`. The clustering is computed on the
listed subset and the resulting assignments are applied to the full dataset,
so excluded variables are aggregated but have no influence on assignment.
This is genuine exclusion, unlike `ClusterConfig(weights={var: 0})`, which
tsam clamps up to a minimal tolerable weight (so a 0-weighted variable still
nudges the assignment). `cluster_on` composes with `weights` for relative
importance among the kept variables and guards against weighting an excluded
variable.
- Implement subset-aggregate + `ClusteringResult.apply()` in cluster()
- Add TestClusterOn (8 cases): equivalence to manual subset+apply, subset
selection drives assignments, excluded var retained, no epsilon-clamp
warning, weights combination, and error paths
- Docs: new v6->v7 migration guide (tsam_xarray backend); strip misplaced
tsam_xarray content out of the v5->v6 guide; register v7 in nav
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 53 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 (1)
📝 WalkthroughWalkthroughAdds Changescluster_on implementation and documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant TransformAccessor
participant tsam_xarray
Caller->>TransformAccessor: cluster(cluster_on=[...], weights=...)
TransformAccessor->>TransformAccessor: validate cluster_on and weights
TransformAccessor->>tsam_xarray: aggregate(..., cluster_on=cluster_on, weights=weights)
tsam_xarray-->>TransformAccessor: clustering result
TransformAccessor-->>Caller: reduced FlowSystem
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/user-guide/migration-guide-v7.md (1)
37-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: "exact same" is wordy.
Consider replacing "exact same semantics" with "same semantics".
🤖 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 `@docs/user-guide/migration-guide-v7.md` around lines 37 - 40, The migration guide wording in the cluster_on description is a bit wordy; update the phrase in the relevant section of the guide to use “same semantics” instead of “exact same semantics” while keeping the rest of the explanation about clustering on the subset and applying assignments to the full dataset unchanged.Source: Linters/SAST tools
tests/test_clustering/test_integration.py (1)
536-548: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a multi-period
cluster_ontest.All
TestClusterOntests use a single-period, single-scenario system. Thecluster_onpath attransform_accessor.py:1452-1475callsagg_subset.clustering.apply()without explicitslice_dims, unlikeapply_clustering()which reconstructs theClusteringResultwithslice_dims. A test withhas_periods=Trueorhas_scenarios=Truewould verify this path works correctly for multi-slice systems.🤖 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 `@tests/test_clustering/test_integration.py` around lines 536 - 548, Add a multi-period or multi-scenario coverage case for the cluster_on path in TestClusterOn so it exercises the transform.cluster flow on a sliced system, not just the single-period single-scenario helper. The issue is that transform_accessor.py’s cluster_on branch calls agg_subset.clustering.apply() without explicitly passing slice_dims, so add a test using has_periods=True or has_scenarios=True that compares cluster_on behavior against the expected clustered result and verifies assignments remain correct across slices. Use the existing test_subset_selection_drives_assignments and the _two_var_system helper as the entry point for locating the behavior.
🤖 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 61-69: The `cluster_inputs()` links use an invalid fragment that
does not match the generated heading anchor, so update the markdown in the
migration guide to use the exact mkdocs anchor for `transform.cluster_inputs()`
or explicitly define a stable anchor on that heading. Check both occurrences of
the `cluster_inputs()` reference and make them point to the same valid target
used by the `cluster_inputs` section.
In `@flixopt/transform_accessor.py`:
- Around line 1397-1419: Add early validation in the cluster_on handling block
to reject any extremes configured via ExtremeConfig that reference variables not
present in cluster_on. Reuse the existing validation pattern near
weights/cluster_on in transform_accessor.py: inspect the extremes passed through
tsam_kwargs_full before calling tsam_xarray.aggregate(), compare their
min_value/max_value variable names against cluster_on_set, and raise a clear
ValueError listing the excluded names and telling the caller to either remove
them from extremes or add them to cluster_on.
---
Nitpick comments:
In `@docs/user-guide/migration-guide-v7.md`:
- Around line 37-40: The migration guide wording in the cluster_on description
is a bit wordy; update the phrase in the relevant section of the guide to use
“same semantics” instead of “exact same semantics” while keeping the rest of the
explanation about clustering on the subset and applying assignments to the full
dataset unchanged.
In `@tests/test_clustering/test_integration.py`:
- Around line 536-548: Add a multi-period or multi-scenario coverage case for
the cluster_on path in TestClusterOn so it exercises the transform.cluster flow
on a sliced system, not just the single-period single-scenario helper. The issue
is that transform_accessor.py’s cluster_on branch calls
agg_subset.clustering.apply() without explicitly passing slice_dims, so add a
test using has_periods=True or has_scenarios=True that compares cluster_on
behavior against the expected clustered result and verifies assignments remain
correct across slices. Use the existing test_subset_selection_drives_assignments
and the _two_var_system helper as the entry point for locating the behavior.
🪄 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: 1dd8582e-76cf-47fc-9673-866ba132b0ae
📒 Files selected for processing (5)
docs/user-guide/migration-guide-v6.mddocs/user-guide/migration-guide-v7.mdflixopt/transform_accessor.pymkdocs.ymltests/test_clustering/test_integration.py
Comment on lines
+1397
to
+1419
| # Pass user-specified weights to tsam_xarray (validates unknown keys). | ||
| # cluster_on filters *which* variables the clustering runs on; weights set the | ||
| # relative importance among the kept ones. Excluded variables are left out of the | ||
| # aggregation entirely (below) and only receive the resulting assignments — this | ||
| # is true exclusion, unlike a 0 weight, which tsam clamps up to a tiny epsilon. | ||
| weights = dict(cluster.weights) if (cluster is not None and cluster.weights is not None) else {} | ||
| if cluster_on is not None: | ||
| if not cluster_on: | ||
| raise ValueError('cluster_on must list at least one variable to cluster on.') | ||
| clusterable = list(ds_for_clustering.data_vars) | ||
| unknown = [name for name in cluster_on if name not in clusterable] | ||
| if unknown: | ||
| raise ValueError( | ||
| f'cluster_on contains variables that are not clusterable inputs: {unknown}. ' | ||
| f'Call transform.cluster_inputs() to list the valid names.' | ||
| ) | ||
| cluster_on_set = set(cluster_on) | ||
| masked = [name for name in weights if name not in cluster_on_set] | ||
| if masked: | ||
| raise ValueError( | ||
| f'ClusterConfig(weights=...) sets weights for variables excluded by cluster_on: ' | ||
| f'{masked}. Remove them from weights or add them to cluster_on.' | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing validation for extremes when used with cluster_on.
The validation block correctly checks for empty cluster_on, unknown variables, and weights referencing excluded variables. However, extremes (via tsam_kwargs_full at line 1425) is passed directly to the subset tsam_xarray.aggregate() call at line 1457. If ExtremeConfig(max_value=[...]) or min_value=[...] references a variable excluded by cluster_on, tsam_xarray will receive data that doesn't contain that variable, likely producing a confusing KeyError rather than a clear validation message.
This is inconsistent with the PR's approach of providing clear, early validation for all cluster_on conflict cases.
🛡️ Suggested validation addition
cluster_on_set = set(cluster_on)
masked = [name for name in weights if name not in cluster_on_set]
if masked:
raise ValueError(
f'ClusterConfig(weights=...) sets weights for variables excluded by cluster_on: '
f'{masked}. Remove them from weights or add them to cluster_on.'
)
+ if extremes is not None:
+ extreme_vars = list(extremes.max_value or []) + list(extremes.min_value or [])
+ excluded_extremes = [v for v in extreme_vars if v not in cluster_on_set]
+ if excluded_extremes:
+ raise ValueError(
+ f'ExtremeConfig references variables excluded by cluster_on: {excluded_extremes}. '
+ f'Remove them from extremes or add them to cluster_on.'
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Pass user-specified weights to tsam_xarray (validates unknown keys). | |
| # cluster_on filters *which* variables the clustering runs on; weights set the | |
| # relative importance among the kept ones. Excluded variables are left out of the | |
| # aggregation entirely (below) and only receive the resulting assignments — this | |
| # is true exclusion, unlike a 0 weight, which tsam clamps up to a tiny epsilon. | |
| weights = dict(cluster.weights) if (cluster is not None and cluster.weights is not None) else {} | |
| if cluster_on is not None: | |
| if not cluster_on: | |
| raise ValueError('cluster_on must list at least one variable to cluster on.') | |
| clusterable = list(ds_for_clustering.data_vars) | |
| unknown = [name for name in cluster_on if name not in clusterable] | |
| if unknown: | |
| raise ValueError( | |
| f'cluster_on contains variables that are not clusterable inputs: {unknown}. ' | |
| f'Call transform.cluster_inputs() to list the valid names.' | |
| ) | |
| cluster_on_set = set(cluster_on) | |
| masked = [name for name in weights if name not in cluster_on_set] | |
| if masked: | |
| raise ValueError( | |
| f'ClusterConfig(weights=...) sets weights for variables excluded by cluster_on: ' | |
| f'{masked}. Remove them from weights or add them to cluster_on.' | |
| ) | |
| # Pass user-specified weights to tsam_xarray (validates unknown keys). | |
| # cluster_on filters *which* variables the clustering runs on; weights set the | |
| # relative importance among the kept ones. Excluded variables are left out of the | |
| # aggregation entirely (below) and only receive the resulting assignments — this | |
| # is true exclusion, unlike a 0 weight, which tsam clamps up to a tiny epsilon. | |
| weights = dict(cluster.weights) if (cluster is not None and cluster.weights is not None) else {} | |
| if cluster_on is not None: | |
| if not cluster_on: | |
| raise ValueError('cluster_on must list at least one variable to cluster on.') | |
| clusterable = list(ds_for_clustering.data_vars) | |
| unknown = [name for name in cluster_on if name not in clusterable] | |
| if unknown: | |
| raise ValueError( | |
| f'cluster_on contains variables that are not clusterable inputs: {unknown}. ' | |
| f'Call transform.cluster_inputs() to list the valid names.' | |
| ) | |
| cluster_on_set = set(cluster_on) | |
| masked = [name for name in weights if name not in cluster_on_set] | |
| if masked: | |
| raise ValueError( | |
| f'ClusterConfig(weights=...) sets weights for variables excluded by cluster_on: ' | |
| f'{masked}. Remove them from weights or add them to cluster_on.' | |
| ) | |
| if extremes is not None: | |
| extreme_vars = list(extremes.max_value or []) + list(extremes.min_value or []) | |
| excluded_extremes = [v for v in extreme_vars if v not in cluster_on_set] | |
| if excluded_extremes: | |
| raise ValueError( | |
| f'ExtremeConfig references variables excluded by cluster_on: {excluded_extremes}. ' | |
| f'Remove them from extremes or add them to cluster_on.' | |
| ) |
🤖 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 1397 - 1419, Add early validation
in the cluster_on handling block to reject any extremes configured via
ExtremeConfig that reference variables not present in cluster_on. Reuse the
existing validation pattern near weights/cluster_on in transform_accessor.py:
inspect the extremes passed through tsam_kwargs_full before calling
tsam_xarray.aggregate(), compare their min_value/max_value variable names
against cluster_on_set, and raise a clear ValueError listing the excluded names
and telling the caller to either remove them from extremes or add them to
cluster_on.
tsam_xarray 0.6.1 adds aggregate(cluster_on=), so cluster_on is now passed
straight through instead of doing the subset-aggregate + apply dance in
flixopt. Same semantics (cluster on the subset, apply assignments to the full
set), upstream also validates extremes referencing excluded variables.
- Bump tsam_xarray pin to >= 0.6.1 (and dev pin to ==0.6.1)
- Replace the subset+apply branch with cluster_on= on the single aggregate call
- Docs: stable { #cluster-inputs } anchor + fix the two cluster_inputs() links;
drop "exact same" -> "same semantics"
- Tests: add multiperiod per-slice cluster_on coverage
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
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 (1)
docs/user-guide/migration-guide-v7.md (1)
28-29: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument
tsam_xarray >= 0.6.1docs/user-guide/migration-guide-v7.md:28-29says>= 0.6.0, butpyproject.tomlpinstsam_xarrayto0.6.1. Align the documented minimum with the supported version.🤖 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 `@docs/user-guide/migration-guide-v7.md` around lines 28 - 29, Update the migration guide dependency note for tsam_xarray so the documented minimum matches the version actually supported by the project. In the migration-guide-v7 content, adjust the tsam_xarray requirement line to reflect the 0.6.1 minimum used in pyproject.toml, while leaving the tsam dependency note unchanged.
🤖 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 `@docs/user-guide/migration-guide-v7.md`:
- Around line 28-29: Update the migration guide dependency note for tsam_xarray
so the documented minimum matches the version actually supported by the project.
In the migration-guide-v7 content, adjust the tsam_xarray requirement line to
reflect the 0.6.1 minimum used in pyproject.toml, while leaving the tsam
dependency note unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b95cc71-a3e0-4769-82d3-1df124907588
📒 Files selected for processing (4)
docs/user-guide/migration-guide-v7.mdflixopt/transform_accessor.pypyproject.tomltests/test_clustering/test_integration.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_clustering/test_integration.py
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.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
1 participant
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
Adds
transform.cluster(cluster_on=[...])— "cluster on these variables only",restoring the ergonomics of the removed v5/v6
data_vars. The clustering is computedon the listed subset and the assignments are applied to the full dataset, so excluded
variables are aggregated but have no influence on the assignment.
Built on tsam_xarray 0.6.1, which adds
aggregate(cluster_on=): flixopt delegatesstraight to it (single
aggregate()call, no subset+apply plumbing on our side).Why not
weights={var: 0}?A
0weight is not genuine exclusion — tsam clamps it up to a minimal tolerableweighting (with a warning), so the variable still nudges the assignment, and you'd have
to enumerate every column to zero the rest.
cluster_onis a hard structural selection;weightsstays purely about relative importance among the kept variables.cluster_oncomposes withClusterConfig(weights=...)(weights may not reference anexcluded variable). Not a performance feature — subset-then-apply is a second pass, on
par with a full clustering.
Dependency
tsam_xarray >= 0.6.1(was>= 0.6.0); dev pin== 0.6.1. 0.6.1 addsaggregate(cluster_on=)and validatesextremesthat reference excluded variables.Docs
migration-guide-v7.md(v6→v7 tsam_xarray backend); thedata_varssection leadswith
cluster_on. Stable{ #cluster-inputs }anchor for thecluster_inputs()links.migration-guide-v6.md: removed tsam_xarray content that had been misplaced into thev5→v6 guide (it shipped in v7); cross-linked to v7.
Tests
TestClusterOn(9 cases): equivalence to a manual subset+apply, subset selection drivesassignments, multiperiod per-slice correctness, excluded var retained, no
epsilon-clamp warning, weights combination, and error paths. Full clustering suite: 252
passed. ruff clean.
Review-finding resolutions
cluster_inputs()doc anchor: added explicit{ #cluster-inputs }andrepointed both links (robust across slugify configs; the emoji made the auto-slug fragile).
0.6.1 already raises a clear
ValueError("extremes references coordinates {...} thatcluster_on excluded").
Note
tsam_xarray 0.6.1 is released (GitHub tag
v0.6.1); at time of writing PyPI was stillingesting it. Local dev/CI resolves once it lands on PyPI.
🤖 Generated with Claude Code
Summary by CodeRabbit
cluster_onoption to control which time-varying inputs define clustering assignments, while retaining other time-varying variables in the reduced output via aggregation.cluster_on, including multi-period consistency and error handling.tsam_xarrayversion used for optional installs.