feat: support tsam v4 (cluster_counts rename, ClusterConfig.weights removal)#105
Merged
Conversation
…emoval) Port the wrapper to run on both released tsam 3.4.x and the upcoming v4 (develop-v4). Two v4 API breaks are the entire delta: - AggregationResult.cluster_weights -> cluster_counts. Read from tsam via a version-compat accessor (new name, fall back to old). Forward our own public attribute to cluster_counts and keep cluster_weights as a deprecated alias emitting FutureWarning. - ClusterConfig.weights removed (constructing it raises TypeError on v4). The deprecation validator now reads the field via getattr so it is a no-op on v4 instead of raising AttributeError. Verified: full suite passes on both tsam 3.4.1 and develop-v4. Floor stays tsam>=3.4.0 (v4 unreleased; dual-version support). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe aggregation result API now uses ChangesCluster Counts Migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (1)
test/test_parametrized.py (1)
302-307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the stale test name.
The body now validates
cluster_counts, buttest_cluster_occurrences_matches_weightsstill reports the old contract. Rename it totest_cluster_occurrences_matches_counts.🤖 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 `@test/test_parametrized.py` around lines 302 - 307, Rename the test method test_cluster_occurrences_matches_weights to test_cluster_occurrences_matches_counts, leaving its implementation and assertions 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.
Inline comments:
In `@docs/examples/getting-started.ipynb`:
- Around line 110-111: Complete the cluster_weights-to-cluster_counts
terminology migration: in docs/examples/getting-started.ipynb lines 110-111,
rename the displayed “weights” label and the to_dataframe column to “count” or
“cluster_counts”; in docs/examples/multi-dim.ipynb line 118, rename the “weight”
column similarly; and in test/test_parametrized.py lines 302-307, rename
test_cluster_occurrences_matches_weights to a count-oriented name.
In `@src/tsam_xarray/_result.py`:
- Around line 115-133: Complete the Google-style docstrings at
src/tsam_xarray/_result.py lines 115-133 by adding Returns sections for
cluster_weights and n_clusters; at src/tsam_xarray/_core.py lines 22-33, add
Args and Returns sections for _cluster_counts; and at src/tsam_xarray/_core.py
lines 195-201, add Args and Raises sections for
_validate_no_cluster_config_weights, documenting each parameter, return value,
and raised exception accurately.
---
Nitpick comments:
In `@test/test_parametrized.py`:
- Around line 302-307: Rename the test method
test_cluster_occurrences_matches_weights to
test_cluster_occurrences_matches_counts, leaving its implementation and
assertions unchanged.
🪄 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 Plus
Run ID: 473aa3dc-3cbc-4f16-9642-a67d8b0b914b
📒 Files selected for processing (9)
docs/data-model.mddocs/examples/getting-started.ipynbdocs/examples/multi-dim.ipynbdocs/index.mdsrc/tsam_xarray/_clustering.pysrc/tsam_xarray/_core.pysrc/tsam_xarray/_result.pytest/test_aggregate.pytest/test_parametrized.py
Comment on lines
110
to
+111
| "print(\"Cluster weights (days each represents):\")\n", | ||
| "result.cluster_weights.to_dataframe(\"weight\")" | ||
| "result.cluster_counts.to_dataframe(\"weight\")" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Finish the cluster_weights → cluster_counts terminology migration. The changed code uses counts, but user-facing notebook output and a test name still expose the deprecated “weights” wording.
docs/examples/getting-started.ipynb#L110-L111: rename the label and.to_dataframe()column fromweighttocountorcluster_counts.docs/examples/multi-dim.ipynb#L118-L118: rename the.to_dataframe("weight")column.test/test_parametrized.py#L302-L307: renametest_cluster_occurrences_matches_weightsto a count-oriented name.
📍 Affects 3 files
docs/examples/getting-started.ipynb#L110-L111(this comment)docs/examples/multi-dim.ipynb#L118-L118test/test_parametrized.py#L302-L307
🤖 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/examples/getting-started.ipynb` around lines 110 - 111, Complete the
cluster_weights-to-cluster_counts terminology migration: in
docs/examples/getting-started.ipynb lines 110-111, rename the displayed
“weights” label and the to_dataframe column to “count” or “cluster_counts”; in
docs/examples/multi-dim.ipynb line 118, rename the “weight” column similarly;
and in test/test_parametrized.py lines 302-307, rename
test_cluster_occurrences_matches_weights to a count-oriented name.
Comment on lines
+115
to
+133
| @property | ||
| def cluster_weights(self) -> xr.DataArray: | ||
| """Deprecated alias for `cluster_counts`. | ||
|
|
||
| Renamed to match tsam v4, where the values are occurrence counts | ||
| rather than weights. Will be removed in a future release. | ||
| """ | ||
| warnings.warn( | ||
| "AggregationResult.cluster_weights is deprecated; use " | ||
| "cluster_counts instead.", | ||
| FutureWarning, | ||
| stacklevel=2, | ||
| ) | ||
| return self.cluster_counts | ||
|
|
||
| @property | ||
| def n_clusters(self) -> int: | ||
| """Number of cluster representative clusters.""" | ||
| return int(self.cluster_weights.sizes[self.dim_names.cluster]) | ||
| return int(self.cluster_counts.sizes[self.dim_names.cluster]) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Complete the changed docstrings with the required Google-style sections. The same documentation-contract gap appears across the changed source files.
src/tsam_xarray/_result.py#L115-L133: addReturns:sections forcluster_weightsandn_clusters.src/tsam_xarray/_core.py#L22-L33: addArgs:andReturns:for_cluster_counts.src/tsam_xarray/_core.py#L195-L201: addArgs:andRaises:for_validate_no_cluster_config_weights.
As per coding guidelines, src/**/*.py docstrings must use Google-style sections where applicable.
📍 Affects 2 files
src/tsam_xarray/_result.py#L115-L133(this comment)src/tsam_xarray/_core.py#L22-L33src/tsam_xarray/_core.py#L195-L201
🤖 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 `@src/tsam_xarray/_result.py` around lines 115 - 133, Complete the Google-style
docstrings at src/tsam_xarray/_result.py lines 115-133 by adding Returns
sections for cluster_weights and n_clusters; at src/tsam_xarray/_core.py lines
22-33, add Args and Returns sections for _cluster_counts; and at
src/tsam_xarray/_core.py lines 195-201, add Args and Raises sections for
_validate_no_cluster_config_weights, documenting each parameter, return value,
and raised exception accurately.
Source: Coding guidelines
This was referenced Jul 23, 2026
FBumann
added a commit
that referenced
this pull request
Jul 23, 2026
Follow-up to #105. Rename the leftover user-facing "weight" labels and column names to "count", and the matching test, so nothing displays the old term: - getting-started.ipynb: "Cluster weights" print label and the to_dataframe("weight") column -> "count". - multi-dim.ipynb: prose "Cluster weights" and to_dataframe("weight") column -> "count". - test_parametrized: test_cluster_occurrences_matches_weights -> ..._matches_counts. Skipped the reviewer's docstring-section suggestions (Returns on the cluster_weights/n_clusters properties; Args/Returns/Raises on _cluster_counts and _validate_no_cluster_config_weights): the codebase convention is prose one-liners for trivial properties and private helpers, with structured sections only on the public aggregate() and public methods that take real parameters. Adding them would break that local consistency. Co-authored-by: Claude Opus 4.8 <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
Makes the wrapper ready for the upcoming tsam v4 (
develop-v4, 4.0.0) while staying green on the currently released tsam 3.4.x.I installed
develop-v4and ran our full suite against it — every failure traced to just two v4 API breaks:AggregationResult.cluster_weights→cluster_counts. The values were always occurrence counts, not weights, so tsam renamed the attribute._cluster_counts: new name, fall back to old).cluster_countsand keepcluster_weightsas a deprecated property alias (FutureWarning).ClusterConfig.weightsremoved (constructingClusterConfig(weights=...)now raisesTypeErroron v4). Our deprecation validator reads the field viagetattr(..., "weights", None), so it is a no-op on v4 instead ofAttributeError-ing.Everything else the migration guide flags (removed
TimeSeriesAggregation/LegacyAPIWarning/MIN_WEIGHT, verbose representation names, column-order change) — we don't touch, and roundtrip/reconstruction tests confirm it.Changes
_result.py— rename public fieldcluster_weights→cluster_counts; add deprecatedcluster_weightsalias property._core.py/_clustering.py—_cluster_counts()compat shim; update all threeAggregationResult(...)construction sites (incl. the slice-merge path); make theClusterConfig.weightsvalidator v4-safe.cluster_counts; add a deprecated-alias test; relax theClusterConfig.weightsrejection test to accept v4'sTypeError.cluster_weights→cluster_counts.Verification
ruff check,ruff format,mypy --strictall clean.Notes
tsam>=3.4.0— the code now works on both v3.4.x and v4, and v4 isn't released yet, so bumping would break installs today.preserve_n_clusters/extreme_replacementsfeature-detection hooks can only be exercised end-to-end once the wrapper runs on v4. (Notedevelop-v4does not yet carry tsam #414, so those flags still no-op there.)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
cluster_counts.Bug Fixes
Documentation
cluster_counts.Deprecations
cluster_weightsremains available as a deprecated alias and now issues a warning when accessed.