fix: drop stored weights on apply() when columns are absent#100
Conversation
tsam's ClusteringResult.apply() hard-fails when a stored weight column is missing from the new data, blocking transfer of a clustering to a differently-composed dataset. At apply time cluster assignments and centers are already fixed, so weights cannot change selection — their only remaining effect is on the weighted accuracy metrics. When a weighted column is absent we now drop all weights and warn instead of propagating the ValueError. A canary test asserts raw tsam still raises; it flips red once the upstream fix (FZJ-IEK3-VSA/tsam#396) lands, signalling this workaround can be removed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe clustering application path now detects missing stored weight columns, emits a warning, and applies the clustering without weights. Tests cover missing-column handling, complete-column transfers, and unchanged upstream ChangesWeighted clustering transfer
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tsam_xarray/_clustering.py (1)
566-578: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required Google-style docstring sections.
Add applicable
Args:andReturns:sections forcr,df, and the returned clustering result.As per coding guidelines, use Google-style docstrings with
Args:,Returns:,Raises:,Attributes:sections (not NumPy style).🤖 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/_clustering.py` around lines 566 - 578, Add Google-style `Args:` entries documenting the `cr` and `df` parameters, and a `Returns:` entry documenting the clustering result returned by this workaround. Keep the existing behavior and explanatory text unchanged, and use the surrounding function’s actual parameter and return types or descriptions.Source: Coding guidelines
🤖 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 `@src/tsam_xarray/_clustering.py`:
- Around line 566-578: Add Google-style `Args:` entries documenting the `cr` and
`df` parameters, and a `Returns:` entry documenting the clustering result
returned by this workaround. Keep the existing behavior and explanatory text
unchanged, and use the surrounding function’s actual parameter and return types
or descriptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec54c565-9493-4a29-a582-4e6694fab953
📒 Files selected for processing (2)
src/tsam_xarray/_clustering.pytest/test_aggregate.py
What
ClusteringResult.apply()(transfer path) now drops the stored clustering weights — with aUserWarning— when a weighted column is absent from the new data, instead of propagating tsam'sValueError: Weight columns not found in data.Why
At apply time the cluster assignments and centers are already fixed, so stored weights cannot change which periods are selected. Their only remaining effect is on the weighted accuracy metrics. tsam nonetheless validates the stored weight columns against the new data and hard-fails, which blocks a legitimate use case: transferring a clustering built on a subset to a differently-composed dataset.
Verified empirically — stripping the weights leaves
cluster_representativesandreconstructedbyte-identical; onlyweighted_rmse/weighted_maechange (and those weights referenced columns not even present in the new data).This is the symmetric counterpart of the already-fixed tsam #276 (extra columns) — here the new data is missing a weighted column.
Changes
_drop_missing_weights()in_clustering.py, called from_apply_single(). Drops all weights when any are missing (an all-or-nothing choice — a partial re-weighting would silently diverge from the source intent, and the core outputs don't move either way). No-ops when all weight columns are present, so same-composition transfers keep their weighted metrics and emit no warning.TestApplyWeightTransfer: warns-and-succeeds, no-warning-when-present, plus a canary (test_upstream_still_rejects_missing_weight_columns) that asserts raw tsam still raises. When it flips red, the upstream fix has landed and this workaround can be removed.Upstream
Tracking: FZJ-IEK3-VSA/tsam#396
🤖 Generated with Claude Code
Summary by CodeRabbit