fix: filter user-provided clustering weights to available columns#625
fix: filter user-provided clustering weights to available columns#625
Conversation
When users pass a ClusterConfig with explicit weights, those weights may reference time series that were removed (e.g., constant arrays dropped before clustering). tsam raises errors on extra weights. Now both user-provided and auto-calculated weights are filtered to only include columns present in the clustering DataFrame. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors clustering weight resolution by adding an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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)
flixopt/transform_accessor.py (1)
1776-1780: Please add a regression test for extra user weight keys.This call path is the right place for the fix; adding one automated test for
ClusterConfig(weights=...)containing keys absent fromdf_for_clustering.columns(especially with constant-array dropping and multi-slice clustering) would lock the behavior in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flixopt/transform_accessor.py` around lines 1776 - 1780, Add a regression test that ensures extra keys in ClusterConfig(weights=...) that are not present in df_for_clustering.columns are ignored/filtered out when building cluster_config via _calculate_clustering_weights and _build_cluster_config_with_weights; specifically construct a test that passes a ClusterConfig with extra weight keys, exercise the same call path around df_for_clustering used in transform_accessor (including a case where some columns are constant and dropped, and a multi-slice clustering scenario) and assert the resulting cluster_config only contains weights for available/non-constant columns and that no exception is raised for unknown keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@flixopt/transform_accessor.py`:
- Around line 1776-1780: Add a regression test that ensures extra keys in
ClusterConfig(weights=...) that are not present in df_for_clustering.columns are
ignored/filtered out when building cluster_config via
_calculate_clustering_weights and _build_cluster_config_with_weights;
specifically construct a test that passes a ClusterConfig with extra weight
keys, exercise the same call path around df_for_clustering used in
transform_accessor (including a case where some columns are constant and
dropped, and a multi-slice clustering scenario) and assert the resulting
cluster_config only contains weights for available/non-constant columns and that
no exception is raised for unknown keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5830606-0044-4d77-b928-99e07c45e14b
📒 Files selected for processing (1)
flixopt/transform_accessor.py
Three tests covering extra keys in ClusterConfig.weights being filtered: - Extra keys not in FlowSystem - Keys for constant (dropped) columns - Multi-period clustering with extra keys Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
ClusterConfigwith explicit weights tocluster(), those weights may reference time series that were removed before clustering (e.g., constant arrays dropped bydrop_constant_arrays)available_columnsparameter in_build_cluster_config_with_weightsTest plan
ClusterConfig(weights={...})with extra keys tocluster()on a multi-period system🤖 Generated with Claude Code
Summary by CodeRabbit