refactor!: strip Clustering to minimal wrapper#655
Merged
Conversation
…lude_original_data BREAKING CHANGES: - Remove ClusteringPlotAccessor (compare, heatmap, clusters methods) - Remove original_data and aggregated_data from Clustering - Remove _metrics from Clustering (use aggregation_result.accuracy instead) - Remove include_original_data parameter from to_dataset/to_netcdf - Simplify _create_reference_structure to just serialize ClusteringResult dict - Remove build_metrics from _ReducedFlowSystemBuilder Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
FBumann
added a commit
that referenced
this pull request
Jul 7, 2026
* refactor: replace per-slice tsam loops with single tsam_xarray.aggregate() call
tsam_xarray handles (period, scenario) slicing internally, eliminating
manual iteration, expand_dims/combine helpers, and per-key DataFrame
conversion. Simplifies _ReducedFlowSystemBuilder, ClusteringResults.apply(),
and the main cluster/apply_clustering methods.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor: remove dead code and fix outdated docstrings after tsam_xarray migration
- Remove broken Clustering.sel() (ignored args, use aggregation_result instead)
- Remove unused _from_aggregation_result classmethod
- Fix no-op rename({'cluster': 'cluster'}) in build_typical_periods
- Update docstrings: tsam 3.0 → tsam_xarray, DataFrame → DataArray
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor!: remove ClusteringResults, delegate to tsam_xarray ClusteringInfo
BREAKING: ClusteringResults class removed. Clustering now delegates all
structure properties (cluster_assignments, cluster_occurrences, segment_*,
dims, coords, etc.) directly to tsam_xarray.ClusteringInfo.
- Remove ClusteringResults class (~450 lines) and all _build_property_array machinery
- Remove AggregationResults backwards-compat alias
- Remove get_result(), cluster_start_positions, timesteps_per_period
- Serialization uses ClusteringInfo.to_dict()/from_dict() (tsam_xarray>=0.5.1)
- Auto-detect _unrename_map from slice_dims for deserialization
- Update tests: remove TestClusteringResults, simplify test fixtures
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address PR review feedback
- Add TODO comments for object.__setattr__ weight-clearing workaround
- Add tsam as explicit dependency (directly imported for ClusterConfig etc.)
- Fix dev version pin: tsam_xarray>=0.5.1 (was >=0.1.1)
- Add assertions for variable dim detection in build_typical_periods/metrics
- Inline _build_property_array into timestep_mapping (single-use helper)
- Document when legacy 'results' kwarg can be removed
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor: eliminate two-step cluster-then-reapply flow
When data_vars is specified, instead of clustering a subset and re-applying
to full data, now all time-varying data goes to tsam_xarray with weight=0
for excluded variables. This removes the object.__setattr__ hack in cluster().
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor!: remove data_vars, auto-weights, and clustering_group/weight from API
BREAKING CHANGES:
- Remove `data_vars` parameter from `cluster()` — use `ClusterConfig(weights={var: 0})`
to exclude variables from influencing clustering instead
- Remove `clustering_group` and `clustering_weight` from `TimeSeriesData` — use
`ClusterConfig(weights={...})` directly
- Remove `_calculate_clustering_weights` and `_build_cluster_config_with_weights`
- Stop dropping constant arrays before clustering — pass all time-dim data to tsam_xarray
- Weight validation now handled by tsam_xarray directly
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor!: strip Clustering API — remove dims, coords, metrics, clustering_data, etc.
BREAKING CHANGES:
- Remove from Clustering: dims, coords, cluster_centers, segment_centers,
n_representatives, representative_weights, metrics, position_within_segment
- Remove clustering_data() from TransformAccessor
- Move position_within_segment computation into _Expander (only consumer)
- Use clustering_info directly for advanced access (dims, coords, etc.)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor: rename clustering_info → clustering_result to match tsam_xarray
Aligns with tsam_xarray's ClusteringResult class name. Renames property,
internal attribute, parameters, and local variables throughout.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor!: replace expand_data/timestep_mapping with tsam_xarray disaggregate()
Replace the entire custom expansion machinery with tsam_xarray's
ClusteringResult.disaggregate(), combined with xarray's built-in
ffill/interpolate_na/fillna for post-processing segmented data.
Removed:
- expand_data(), build_expansion_divisor(), timestep_mapping (Clustering)
- _build_timestep_mapping() helper function
- _interpolate_charge_state_segmented(), _expand_first_timestep_only() (_Expander)
- _compute_position_within_segment() (_Expander)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor!: remove legacy variable category fallback from _Expander
Remove backwards-compat pattern matching (_is_state_variable,
_is_first_timestep_variable, _build_segment_total_varnames) that was
only needed for FlowSystems saved before _variable_categories was added
in v6.0. Now requires _variable_categories to always be present.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor!: remove ClusteringPlotAccessor, metrics, original_data, include_original_data (#655)
BREAKING CHANGES:
- Remove ClusteringPlotAccessor (compare, heatmap, clusters methods)
- Remove original_data and aggregated_data from Clustering
- Remove _metrics from Clustering (use aggregation_result.accuracy instead)
- Remove include_original_data parameter from to_dataset/to_netcdf
- Simplify _create_reference_structure to just serialize ClusteringResult dict
- Remove build_metrics from _ReducedFlowSystemBuilder
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* test: add expansion regression tests for cluster → optimize → expand pipeline
Verifies numerical equivalence of expanded solution values for both
non-segmented and segmented clustering, including flow rates, costs,
objectives, shapes, and NaN-free expansion.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* docs!: consolidate clustering notebooks, remove obsolete ones
BREAKING: Remove 3 clustering notebooks, update 1:
- Remove 08d-clustering-multiperiod.ipynb (tsam_xarray handles this)
- Remove 08e-clustering-internals.ipynb (internals no longer exist)
- Remove 08f-clustering-segmentation.ipynb (just a parameter, covered in 08c)
- Update 08c-clustering.ipynb: remove clustering_data, data_vars, plot.compare,
metrics sections; add tsam_xarray links; update API reference
5 clustering notebooks → 2 (08c + 08c2-storage-modes)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: add temporal_resolution to reserved_tsam_keys guard
Prevents users from silently overriding the computed timestep size
via tsam_kwargs.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* docs: remove links to notebooks that don't exist yet
Remove references to 08d-clustering-multiperiod, 08e-clustering-internals,
and 08f-clustering-segmentation notebooks that haven't been created yet.
These caused mkdocs --strict to abort with broken link warnings.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor: address PR review and adapt to tsam_xarray 0.6.0
Bump pins to tsam_xarray>=0.6.0 (handles time_coords natively) and
tsam>=3.4.0 (apply() respects original weights). Drop the corresponding
workarounds: the object.__setattr__(time_coords) hack in Clustering and
the weight-clearing loop in apply_clustering.
Other review follow-ups:
- Remove the legacy `results=` kwarg from Clustering.__init__ and the
matching fallback in from_json — pre-refactor netcdfs can't load anyway.
- Strip dead clustering_arrays plumbing in io.py (always empty post-refactor).
- Remove no-op rename(cluster='cluster') in build_cluster_weights.
- Replace bare assert in build_typical_periods with ValueError.
New: transform.cluster_inputs() returns the Dataset of variables that
cluster() will pass to tsam (every time-dim var, constants included). Use
to enumerate columns for ClusterConfig(weights={...}). Different from v5's
clustering_data() — not a rename, the v5 set isn't meaningful anymore.
Tests:
- TestClusterInputs: enumeration, constants, documented weight-zero pattern,
drift guard between cluster_inputs() and what cluster() feeds tsam.
- test_dim_names_order: Clustering.dim_names == ['period', 'scenario'] for
period+scenario FlowSystems.
- test_cluster_with_constant_columns: clustering must not crash on
zero-variance columns now that drop_constant_arrays is gone.
- Strengthened test_clustering_result_preserved_after_load with deeper
equality on cluster_assignments / cluster_occurrences / dim_names.
- test_expansion_regression: derive reference values from fixture parameters
(mean*hours/eta*price) instead of pinned magic numbers.
Docs:
- Migration guide: removed-API summary table, new cluster_inputs() entry,
default-weight-is-1.0 callout, netcdf-incompat note.
- Notebook 08c: callout explaining which variables get clustered by default
and how to scope clustering via ClusterConfig(weights={...}).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(clustering): correct segmented state expansion and single-slice extremes guard
Two defects found while reviewing the tsam_xarray migration:
- Segmented state variables (e.g. Storage charge_state) were expanded with a
global interpolate_na, which interpolated across period boundaries and left
the final period's last segment as NaN in the solution. Replace with a
segment-aware linear interpolation between the (loss-correct) segment boundary
values, matching the pre-refactor behavior. No NaN, no cross-period bleed.
- The ExtremeConfig guard rejected any system with a period/scenario dimension,
including length-1 indices that produce a single clustering slice where any
extremes method is valid. Gate on slice count (n_periods * n_scenarios > 1)
instead of presence.
Adds regression tests: segmented + storage expand has no NaN in charge_state;
single-period new_cluster is allowed while genuine multi-period still requires
method='replace'.
* style: ruff format test_clustering/test_integration.py
---------
Co-authored-by: Claude Opus 4.6 (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
Builds on #654. Further strips the Clustering class to a minimal wrapper around tsam_xarray.
ClusteringPlotAccessorentirely (~370 lines) — users can plot with tsam_xarray's DataArrays directlyoriginal_data,aggregated_data,_metricsfrom Clustering — no longer stored or serializedinclude_original_dataparameter fromto_dataset()/to_netcdf()build_metrics()from_ReducedFlowSystemBuilder_create_reference_structure()to just serializeClusteringResult.to_dict()+original_timestepsxrandnpimports to TYPE_CHECKING (no longer needed at runtime in base.py)base.py: 1633 → 393 lines (-76%)Test plan
🤖 Generated with Claude Code