Refactor weights API: always normalize scenario weights #547
Conversation
Changes made:
1. elements.py - Flow tracking:
# Before:
flow_hours = self.flow_rate * self._model.timestep_duration
weighted_flow_hours = flow_hours * self._model.cluster_weight
tracked_expression=weighted_flow_hours.sum(self._model.temporal_dims)
# After:
tracked_expression=self._model.weights.sum_temporal(self.flow_rate)
2. elements.py - Load factor total hours:
# Before:
total_hours = (self._model.timestep_duration * self._model.cluster_weight).sum(self._model.temporal_dims)
# After:
total_hours = self._model.weights.temporal.sum(self._model.weights.temporal_dims)
3. features.py - Status tracking:
# Before:
active_hours = self.status * self._model.timestep_duration
weighted_active_hours = active_hours * self._model.cluster_weight
tracked_expression=weighted_active_hours.sum(self._model.temporal_dims)
# After:
tracked_expression=self._model.weights.sum_temporal(self.status)
4. features.py - Temporal effects summing (only needs cluster weight since already per-timestep):
# Before:
weighted_per_timestep = self.total_per_timestep * self._model.cluster_weight
temporal_dims = [d for d in self.total_per_timestep.dims if d not in ('period', 'scenario')]
# After:
weighted_per_timestep = self.total_per_timestep * self._model.weights.cluster
self._eq_total.lhs -= weighted_per_timestep.sum(dim=self._model.weights.temporal_dims)
📝 WalkthroughWalkthroughTimeSeriesWeights is replaced with a new Weights class that provides a generalized weighting API initialized from FlowSystem. Temporal weighting now uses computed properties and a unified sum_temporal method. All modules using weights are updated to leverage this new interface, eliminating manual weight calculations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
flixopt/statistics_accessor.py (1)
848-852: Use ofself._fs.weights.clustercorrectly aligns with new weights APIMultiplying temporal shares by
weights.clusterand then summing over all non-period/scenario dims preserves the previous semantics while handling both clustered (DataArray) and non‑clustered (scalar 1.0) systems cleanly. No issues seen here.flixopt/__init__.py (1)
34-35: Public export switches fromTimeSeriesWeightstoWeights– consider transition planExposing
Weightsvia__all__is consistent with the new unified weights API, but any external code doingfrom flixopt import TimeSeriesWeightswill now break unless you keep a compatibility alias somewhere.If you want a softer migration path, consider re‑exporting an alias (e.g.
TimeSeriesWeights = Weights) or documenting this as a breaking change in the release notes.Also applies to: 36-62
flixopt/features.py (1)
199-214: StatusModel active‑hours tracking correctly switches tomodel.weightsComputing:
total_hoursfromself._model.weights.temporal.sum(self._model.weights.temporal_dims), andtracked_expressionasself._model.weights.sum_temporal(self.status)gives a consistent, weights‑aware definition of total/active hours that automatically handles clustered and non‑clustered systems. The fallback upper bound
total_hours.max().item()is conservative but safe (it will never cut off feasible solutions since it’s ≥ any period/scenario total).No functional issues here.
flixopt/structure.py (1)
92-100: Consider exposingscenario_weightsthrough a public accessor.The
scenarioproperty accessesself._fs._scenario_weights, a private attribute. While this works, it creates a dependency on internal implementation details. IfFlowSystemhas or could add a publicscenario_weightsproperty (similar toperiod_weights), using that would be more robust.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
flixopt/__init__.pyflixopt/elements.pyflixopt/features.pyflixopt/flow_system.pyflixopt/statistics_accessor.pyflixopt/structure.pytests/test_clustering/test_integration.py
🧰 Additional context used
🧬 Code graph analysis (7)
flixopt/statistics_accessor.py (3)
flixopt/structure.py (2)
weights(329-335)cluster(74-78)flixopt/flow_system.py (1)
weights(2009-2021)flixopt/transform_accessor.py (1)
cluster(577-933)
flixopt/elements.py (2)
flixopt/structure.py (5)
weights(329-335)sum_temporal(102-119)temporal(81-83)temporal_dims(86-90)temporal_dims(318-326)flixopt/flow_system.py (1)
weights(2009-2021)
flixopt/__init__.py (1)
flixopt/structure.py (1)
Weights(46-119)
flixopt/flow_system.py (1)
flixopt/structure.py (2)
Weights(46-119)weights(329-335)
flixopt/features.py (3)
flixopt/structure.py (6)
weights(329-335)temporal(81-83)temporal_dims(86-90)temporal_dims(318-326)sum_temporal(102-119)cluster(74-78)flixopt/flow_system.py (1)
weights(2009-2021)flixopt/modeling.py (2)
ModelingPrimitives(196-395)expression_tracking_variable(200-242)
flixopt/structure.py (1)
flixopt/flow_system.py (1)
weights(2009-2021)
tests/test_clustering/test_integration.py (2)
flixopt/flow_system.py (2)
FlowSystem(56-2377)weights(2009-2021)flixopt/structure.py (11)
Weights(46-119)weights(329-335)time(69-71)cluster(74-78)temporal_dims(86-90)temporal_dims(318-326)temporal(81-83)sum_temporal(102-119)timestep_duration(298-300)timestep_duration(1830-1831)cluster_weight(307-315)
🔇 Additional comments (9)
flixopt/elements.py (1)
680-688: FlowModel now correctly delegates temporal aggregation tomodel.weightsUsing
self._model.weights.sum_temporal(self.flow_rate)fortotal_flow_hoursandself._model.weights.temporal.sum(self._model.weights.temporal_dims)fortotal_hourskeeps the previous behavior (timestep × cluster weighting, then summing over temporal dims) while centralizing the logic in the Weights abstraction. This should behave correctly for both clustered and non‑clustered systems and avoids duplicating weighting formulas.Also applies to: 840-854
flixopt/features.py (1)
628-632: ShareAllocationModel now uses unified cluster/temporal weightingReplacing direct use of
self._model.cluster_weightwith:weighted_per_timestep = self.total_per_timestep * self._model.weights.cluster self._eq_total.lhs -= weighted_per_timestep.sum(dim=self._model.weights.temporal_dims)keeps the previous semantics (time × cluster aggregation to
total) while routing everything through the Weights API. Broadcasting and summation dimensions match the intended temporal aggregation.tests/test_clustering/test_integration.py (3)
8-8: LGTM!Import updated correctly to use the new
Weightsclass from the public API.
11-64: LGTM!The
TestWeightsclass provides good coverage for the new Weights API surface:
- Creation via FlowSystem.weights property
- Individual property accessors (time, cluster, temporal, temporal_dims)
- The
sum_temporalmethod with a clear rate-to-total conversion testThe use of
xr.testing.assert_equalfor DataArray comparisons is appropriate.
67-106: LGTM!The updated tests correctly validate the new Weights API behavior:
- Instance type checking with
Weights- Temporal calculation verification (timestep_duration × cluster_weight)
- Proper use of
xr.testing.assert_equalfor DataArray comparisonThe
test_weights_with_cluster_weighttest appropriately validates per-timestep weighting without full clustering enabled.flixopt/structure.py (4)
46-66: LGTM!The
Weightsclass provides a clean, unified interface for temporal weighting. The delegation pattern toFlowSystemattributes keeps the implementation simple and ensures consistency with the source data.
68-90: LGTM!The properties are well-designed:
timeandclusterdelegate cleanly to FlowSystem attributestemporalis a computed property that correctly multiplies time × clustertemporal_dimsdynamically returns the appropriate dimensions based on clustering status
102-119: LGTM!The
sum_temporalmethod correctly implements the rate-to-total conversion pattern by:
- Multiplying by the combined temporal weight (time × cluster)
- Summing over the appropriate temporal dimensions
The docstring clearly explains the use case with good examples.
328-335: LGTM!The property correctly provides access to the unified weighting system. Creating a new
Weightsinstance on each access is acceptable since the class is lightweight (only stores a reference to the FlowSystem).
| from .clustering import Clustering | ||
| from .solvers import _Solver | ||
| from .structure import TimeSeriesWeights | ||
| from .structure import Weights | ||
| from .types import Effect_TPS, Numeric_S, Numeric_TPS, NumericOrBool | ||
|
|
There was a problem hiding this comment.
FlowSystem.weights correctly exposes the new Weights API, but docstring is stale
The property implementation:
from .structure import Weights
return Weights(self)is the right way to surface the unified weighting system without introducing import cycles.
However, the example in the docstring still shows weights.timestep, while the Weights class exposes time, cluster, temporal, etc. To avoid confusion, the example should be updated, e.g.:
- >>> weights = flow_system.weights
- >>> flow_hours = flow_rate * weights.timestep
- >>> total = weights.sum_temporal(flow_hours)
+ >>> weights = flow_system.weights
+ >>> flow_hours = flow_rate * weights.time
+ >>> total = weights.sum_temporal(flow_hours)Also applies to: 2009-2021
🤖 Prompt for AI Agents
In flixopt/flow_system.py around lines 41 to 45, the FlowSystem.weights property
correctly returns Weights(self) but the docstring example still references the
old weights.timestep API; update the docstring to use the current Weights
attributes (e.g., weights.time, weights.cluster, weights.temporal) and show
usage with the unified Weights instance returned by FlowSystem.weights; also
scan and update other docstring/examples in the file (and related files from
2009-2021) to replace any remaining weights.timestep references with the new
attribute names to avoid confusion.
Changes made:
1. elements.py - Flow tracking:
# Before:
flow_hours = self.flow_rate * self._model.timestep_duration
weighted_flow_hours = flow_hours * self._model.cluster_weight
tracked_expression=weighted_flow_hours.sum(self._model.temporal_dims)
# After:
tracked_expression=self._model.weights.sum_temporal(self.flow_rate)
2. elements.py - Load factor total hours:
# Before:
total_hours = (self._model.timestep_duration * self._model.cluster_weight).sum(self._model.temporal_dims)
# After:
total_hours = self._model.weights.temporal.sum(self._model.weights.temporal_dims)
3. features.py - Status tracking:
# Before:
active_hours = self.status * self._model.timestep_duration
weighted_active_hours = active_hours * self._model.cluster_weight
tracked_expression=weighted_active_hours.sum(self._model.temporal_dims)
# After:
tracked_expression=self._model.weights.sum_temporal(self.status)
4. features.py - Temporal effects summing (only needs cluster weight since already per-timestep):
# Before:
weighted_per_timestep = self.total_per_timestep * self._model.cluster_weight
temporal_dims = [d for d in self.total_per_timestep.dims if d not in ('period', 'scenario')]
# After:
weighted_per_timestep = self.total_per_timestep * self._model.weights.cluster
self._eq_total.lhs -= weighted_per_timestep.sum(dim=self._model.weights.temporal_dims)
FBumann
changed the title
#549) * Enable selecting a single period/scenario * No selected_* tracking * Clustering IO * Clustering IO * Improve IO * Improve validation * Fix cluster weight IO * Fix cluster weights stuff * Fix cluster weights stuff * Refactor weights API: always normalize scenario weights (#547) * Add weights class * Add weights class * The Weights API is now used in the modeling equations: Changes made: 1. elements.py - Flow tracking: # Before: flow_hours = self.flow_rate * self._model.timestep_duration weighted_flow_hours = flow_hours * self._model.cluster_weight tracked_expression=weighted_flow_hours.sum(self._model.temporal_dims) # After: tracked_expression=self._model.weights.sum_temporal(self.flow_rate) 2. elements.py - Load factor total hours: # Before: total_hours = (self._model.timestep_duration * self._model.cluster_weight).sum(self._model.temporal_dims) # After: total_hours = self._model.weights.temporal.sum(self._model.weights.temporal_dims) 3. features.py - Status tracking: # Before: active_hours = self.status * self._model.timestep_duration weighted_active_hours = active_hours * self._model.cluster_weight tracked_expression=weighted_active_hours.sum(self._model.temporal_dims) # After: tracked_expression=self._model.weights.sum_temporal(self.status) 4. features.py - Temporal effects summing (only needs cluster weight since already per-timestep): # Before: weighted_per_timestep = self.total_per_timestep * self._model.cluster_weight temporal_dims = [d for d in self.total_per_timestep.dims if d not in ('period', 'scenario')] # After: weighted_per_timestep = self.total_per_timestep * self._model.weights.cluster self._eq_total.lhs -= weighted_per_timestep.sum(dim=self._model.weights.temporal_dims) * The Weights API is now used in the modeling equations: Changes made: 1. elements.py - Flow tracking: # Before: flow_hours = self.flow_rate * self._model.timestep_duration weighted_flow_hours = flow_hours * self._model.cluster_weight tracked_expression=weighted_flow_hours.sum(self._model.temporal_dims) # After: tracked_expression=self._model.weights.sum_temporal(self.flow_rate) 2. elements.py - Load factor total hours: # Before: total_hours = (self._model.timestep_duration * self._model.cluster_weight).sum(self._model.temporal_dims) # After: total_hours = self._model.weights.temporal.sum(self._model.weights.temporal_dims) 3. features.py - Status tracking: # Before: active_hours = self.status * self._model.timestep_duration weighted_active_hours = active_hours * self._model.cluster_weight tracked_expression=weighted_active_hours.sum(self._model.temporal_dims) # After: tracked_expression=self._model.weights.sum_temporal(self.status) 4. features.py - Temporal effects summing (only needs cluster weight since already per-timestep): # Before: weighted_per_timestep = self.total_per_timestep * self._model.cluster_weight temporal_dims = [d for d in self.total_per_timestep.dims if d not in ('period', 'scenario')] # After: weighted_per_timestep = self.total_per_timestep * self._model.weights.cluster self._eq_total.lhs -= weighted_per_timestep.sum(dim=self._model.weights.temporal_dims) * Minor fixes in test * Improve weighting system and normalization of scenrio weights * Update CHANGELOG.md * 1. ClusterStructure.n_clusters naming - Added explicit rename (matching n_representatives pattern) to avoid "None" variable names in serialized datasets 2. original_timesteps validation - Added explicit KeyError with actionable message when original_time coordinate is missing 3. active_hours bounds simplified - Passing total_hours DataArray directly instead of .max().item() fallback, allowing proper per-(period, scenario) bounds
Summary
Simplify the weights API by always normalizing scenario weights when set on FlowSystem. This removes the need for the
normalize_weightsparameter and ensures consistent behavior across all access paths.Changes
Breaking Changes
.sel(),.transform.sel()) re-normalize weightsAPI Changes
normalize_weightsparameter increate_model(),build_model(),optimize()FlowSystem.weightsnow returnsdict[str, xr.DataArray](no more float fallbacks)FlowSystemDimensionstype now includes'cluster'Bug Fixes
temporal_weightvssum_temporal()implementation inconsistencymodel.weights['scenario']vsmodel.scenario_weightsdivergenceInternal
normalize_weightsfromFlowSystemModelconstructor_unit_weight()helper for consistent unit weight creationFlowSystemModel.scenario_weightsto delegate to FlowSystem