Feature/sums over all periods #475
Conversation
…d for both Effects and Flows: Effects - Renamed parameters: minimum_total → minimum_total_per_period, maximum_total → maximum_total_per_period - New parameters: minimum and maximum for weighted sum across ALL periods - Effect-specific weights: Effects can override FlowSystem weights (e.g., for discounting in costs vs equal weighting for CO2) - Backward compatibility: Deprecation wrappers ensure existing code continues to work Flows - Renamed parameters: flow_hours_total_min/max → flow_hours_per_period_min/max - New parameters: total_flow_hours_min/max for weighted sum across ALL periods - Uses FlowSystem period weights for weighting - Backward compatibility: Deprecation wrappers for old parameter names Test Results - 616/616 tests passing - no regressions introduced - All existing functionality preserved - Weighted sums respect period weights (auto-derived from period index or user-specified) The implementation is production-ready and maintains full backward compatibility with existing code.
…flixopt/structure.py:218-228 where the FlowSystemModel.weights property wasn't normalizing weights despite its docstring claiming it would. The property now correctly normalizes weights to sum to 1 when normalize_weights=True.
… the _over_periods suffix for weighted sums across all periods: Effects (4 parameters): - minimum_total_per_period → minimum_total - maximum_total_per_period → maximum_total - minimum → minimum_over_periods - maximum → maximum_over_periods Flows (4 parameters): - flow_hours_per_period_max → flow_hours_max - flow_hours_per_period_min → flow_hours_min - total_flow_hours_max → flow_hours_max_over_periods - total_flow_hours_min → flow_hours_min_over_periods OnOffParameters (3 parameters): - on_hours_total_min → on_hours_min - on_hours_total_max → on_hours_max - switch_on_total_max → switch_on_max
WalkthroughAdds period-aware weighting and weighted across-period constraints for Effects and Flows; renames several timing parameters with deprecated aliases; moves period-weight computation into FlowSystem (separate Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant E as Element / Effect
participant FS as FlowSystem
participant M as Calculation / ModelBuilder
participant S as Solver
rect rgba(240,248,255,0.9)
U->>E: instantiate (per-period params, optional over-period bounds, optional element period_weights)
E->>FS: attach via _set_flow_system / propagation
E->>E: store attrs (`flow_hours_max`, `period_weights`, `*_over_periods`)
end
Note right of FS: compute period metadata
FS->>FS: _compute_period_metadata() → period_weights, weight_of_last_period
U->>M: call Calculation.solve()
alt not modeled yet
M->>M: run two‑phase modeling (build model → solve)
end
M->>FS: request objective_weights (period_weights × scenario_weights)
alt element has over-period bounds
M->>S: create weighted_sum var and equality constraint: Σ(period_var * period_weight) == total_over_periods ∈ [min,max]
else
M->>S: add per-period constraints using `flow_hours_max` / `on_hours_max`
end
M->>S: assemble objective scaled by objective_weights
S->>U: return solution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/structure.py (1)
219-229: Bug: weights normalization breaks for scalars/lists and lacks zero-sum guard.
flow_system.weightsmay be a scalar/list; calling.sum()will error and dims won’t align. Fit to coords first and handle zero-sum.Apply:
@@ def weights(self) -> int | xr.DataArray: """Returns the weights of the FlowSystem. Normalizes to 1 if normalize_weights is True""" - if self.flow_system.weights is not None: - weights = self.flow_system.weights - if self.normalize_weights: - # Normalize weights to sum to 1 - weights = weights / weights.sum() - return weights - - return self.flow_system.fit_to_model_coords('weights', 1, dims=['period', 'scenario']) + # Prefer provided weights; always align to model coords + if self.flow_system.weights is not None: + weights = self.flow_system.fit_to_model_coords( + 'weights', self.flow_system.weights, dims=['period', 'scenario'] + ) + if self.normalize_weights: + total = weights.sum() + if np.isclose(total, 0): + raise ValueError('FlowSystemModel.weights: weights sum to 0; cannot normalize.') + weights = weights / total + return weights + + # Fallback: uniform weights + return self.flow_system.fit_to_model_coords('weights', 1, dims=['period', 'scenario'])
🧹 Nitpick comments (3)
CHANGELOG.md (1)
54-89: Tighten migration notes for “_over_periods” and deprecations.
- Add 1–2 sentences clarifying weighting: sums “over periods” are weighted by FlowSystem/effect weights.
- State removal timeline for deprecated names (target version).
- Include a short example showing per-period vs over-periods dims to avoid ambiguity.
flixopt/features.py (1)
208-214: Switch count constraint is fine; optional naming tweak.Implementation is correct. Consider renaming variable short_name from
on_hours_totaltoon_hoursin a future pass for consistency with parameter names (keep alias for backward compatibility).flixopt/interface.py (1)
1276-1290: Use instance helper for deprecated kwargs (avoid static call with None “self”).Call
self._handle_deprecated_kwarg(...)directly. Current pattern works but is brittle and confuses static tooling.Apply:
- from .structure import Element # Import here to avoid circular import - - on_hours_min = Element._handle_deprecated_kwarg( - None, kwargs, 'on_hours_total_min', 'on_hours_min', on_hours_min - ) - on_hours_max = Element._handle_deprecated_kwarg( - None, kwargs, 'on_hours_total_max', 'on_hours_max', on_hours_max - ) - switch_on_max = Element._handle_deprecated_kwarg( - None, kwargs, 'switch_on_total_max', 'switch_on_max', switch_on_max - ) + on_hours_min = self._handle_deprecated_kwarg( + kwargs, 'on_hours_total_min', 'on_hours_min', on_hours_min + ) + on_hours_max = self._handle_deprecated_kwarg( + kwargs, 'on_hours_total_max', 'on_hours_max', on_hours_max + ) + switch_on_max = self._handle_deprecated_kwarg( + kwargs, 'switch_on_total_max', 'switch_on_max', switch_on_max + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)flixopt/components.py(1 hunks)flixopt/effects.py(15 hunks)flixopt/elements.py(8 hunks)flixopt/features.py(2 hunks)flixopt/flow_system.py(5 hunks)flixopt/interface.py(10 hunks)flixopt/linear_converters.py(1 hunks)flixopt/structure.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
flixopt/elements.py (3)
flixopt/structure.py (2)
_handle_deprecated_kwarg(414-473)_validate_kwargs(475-503)flixopt/flow_system.py (2)
fit_to_model_coords(604-642)coords(999-1005)flixopt/modeling.py (2)
ModelingPrimitives(193-383)expression_tracking_variable(197-233)
flixopt/features.py (1)
flixopt/structure.py (1)
add_variables(1270-1279)
flixopt/interface.py (2)
flixopt/structure.py (2)
Element(844-891)_handle_deprecated_kwarg(414-473)flixopt/flow_system.py (1)
fit_to_model_coords(604-642)
flixopt/flow_system.py (2)
flixopt/effects.py (1)
weights(527-545)flixopt/structure.py (2)
weights(219-228)hours_of_previous_timesteps(184-185)
flixopt/effects.py (2)
flixopt/structure.py (6)
weights(219-228)_handle_deprecated_kwarg(414-473)add_variables(1270-1279)get_coords(187-216)get_coords(1329-1334)add_constraints(1281-1290)flixopt/flow_system.py (2)
fit_to_model_coords(604-642)coords(999-1005)
flixopt/structure.py (2)
flixopt/effects.py (1)
weights(527-545)flixopt/flow_system.py (1)
fit_to_model_coords(604-642)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (17)
flixopt/features.py (1)
183-191: On/off hour bounds wiring looks correct.Tracked total hours per (period, scenario) with lower/upper from on_hours_min/max is sound.
flixopt/linear_converters.py (1)
419-423: Docs updated toon_hours_max— LGTM.Examples now match the new OnOffParameters naming.
flixopt/interface.py (1)
1321-1329: Code changes are correct — review comment contains function name mismatch.The code calls
fit_to_model_coords(lines 1321-1329), notfit_effects_to_model_coords. The actual function signature accepts positional arguments fornameanddata, followed by optionaldims. The calls correctly use positional arguments for the label and value, then keyword argument fordims. No issues found with the implementation.flixopt/flow_system.py (3)
297-398: Period metadata computation logic is well-structured.The new methods
_create_periods_with_extra,calculate_weight_per_period, and_compute_period_metadatafollow the same pattern as the existing time metadata methods, ensuring consistency. The logic correctly:
- Creates an extra period for weight calculation
- Computes weights from period differences
- Handles None periods gracefully
521-523: Good integration infrom_dataset.The
weight_of_last_periodparameter is correctly passed throughfrom_dataset, ensuring that period metadata is properly reconstructed when loading from a dataset. This maintains consistency with howhours_of_last_timestepandhours_of_previous_timestepsare handled.
184-188: No breaking code patterns found—auto-derivation is backward compatible.The verification shows that all code consuming
weightsuses the property wrapper instructure.py(lines 219-228), which explicitly handles theNonecase by falling back tofit_to_model_coords('weights', 1, ...). The new auto-derivation behavior only affects the specific case where bothweights=Noneandweight_per_period is not None, and this change is safe because existing code expectingNonestill getsNonewhenweight_per_period is None. The property layer ensures no consumer receives aNonevalue without falling back to a default weight of 1.flixopt/elements.py (5)
328-333: Clear documentation of new flow hours parameters.The docstring effectively explains the new parameters:
flow_hours_max/minfor per-period limitsflow_hours_max/min_over_periodsfor limits across all periods- Correctly notes that over_periods constraints are weighted by FlowSystem period weights
461-479: Comprehensive deprecation handling for multiple legacy parameter names.The deprecation logic correctly handles multiple old parameter names mapping to the new unified names:
flow_hours_per_period_maxandflow_hours_total_maxboth map toflow_hours_maxtotal_flow_hours_maxmaps toflow_hours_max_over_periodsThe
_handle_deprecated_kwargmethod properly detects conflicts when multiple deprecated names are used together, preventing ambiguous specifications.
521-532: Correct dimension specifications in transform_data.The dimensions specified for each parameter are correct:
flow_hours_max/min:['period', 'scenario']- appropriate for per-period constraintsflow_hours_max/min_over_periods:['scenario']- appropriate for cross-period constraintsThis ensures proper broadcasting and constraint formulation.
595-715: Thorough backward compatibility through deprecated properties.All deprecated property names are covered with appropriate getter/setter pairs that emit DeprecationWarnings:
flow_hours_per_period_max/min→flow_hours_max/minflow_hours_total_max/min→flow_hours_max/mintotal_flow_hours_max/min→flow_hours_max/min_over_periodsThis provides a smooth migration path for existing code while clearly communicating the preferred API.
752-779: Weighted sum over periods logic is correctly implemented.The implementation properly:
- Gets
weight_per_periodfrom FlowSystem (line 755)- Handles both weighted (lines 756-758) and unweighted (lines 760-761) cases
- Computes weighted total:
(self.total_flow_hours * weight_per_period).sum('period')(line 758)
- This correctly broadcasts the 1D weight_per_period over the 2D total_flow_hours
- Summing over 'period' produces the expected ['scenario'] dimension
- Creates a tracking variable with appropriate bounds and coords (lines 764-778)
The dimension alignment is correct and the logic matches the PR's stated objective of supporting constraints across all periods.
flixopt/effects.py (6)
51-85: Comprehensive documentation of effect weighting and cross-period constraints.The updated docstring clearly explains:
- The new
weightsparameter for effect-specific weighting schemes- The distinction between per-period bounds (
minimum/maximum_total) and cross-period bounds (minimum/maximum_over_periods)- Examples showing both use cases (lines 98-117)
- Proper documentation of all deprecated parameter names
235-246: Correct deprecation handling for renamed effect parameters.The deprecation handling properly maps:
minimum_total_per_period→minimum_totalmaximum_total_per_period→maximum_totalminimum→minimum_over_periodsmaximum→maximum_over_periodsThis maintains backward compatibility while clarifying the semantic difference between per-period and cross-period constraints.
384-462: Complete deprecated property coverage.All renamed parameters have corresponding deprecated property accessors with appropriate warnings:
minimum_total_per_period/maximum_total_per_period(lines 384-422)minimum/maximum(lines 424-462)This ensures existing code continues to work while guiding users toward the new API.
526-545: Effect-specific weights property is well-designed.The
weightsproperty correctly:
- Returns effect-specific weights if defined (lines 538-542)
- Falls back to FlowSystem weights otherwise (line 545)
- Fits effect weights to model coords with
['period', 'scenario']dimensions (line 541)This design allows different effects to have different weighting schemes (e.g., discounting for costs vs. equal weights for emissions) as described in the docstring.
586-600: Weighted sum over periods constraint correctly uses effect weights.The implementation properly:
- Computes weighted total using the effect's
weightsproperty:(self.total * self.weights).sum('period')(line 589)
- This correctly uses effect-specific weights if defined, or FlowSystem weights as fallback
- Creates a tracking variable with
['scenario']coords (line 595)- Applies the appropriate bounds from
minimum/maximum_over_periods(lines 593-594)The dimension handling is correct, and using
self.weightsensures consistency with the effect's weighting scheme.
838-844: Objective calculation correctly uses effect-specific weights.The updated objective formulation:
- Gets weights from the objective effect's
weightsproperty (line 839)- Normalizes if requested (lines 840-841)
- Computes weighted objective:
(... * objective_weights).sum()(line 843)This ensures that the objective respects effect-specific weighting (e.g., temporal discounting for cost effects), which is a key feature of this PR.
| switch_on_max=10, # Maximum 10 starts per day | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
Docs: rename looks consistent with API (switch_on_max).
Good alignment with the new OnOffParameters naming.
Quick sweep for lingering old names:
🏁 Script executed:
#!/bin/bash
rg -nP --glob '!**/site/**' '(on_hours_total_(min|max)|switch_on_total_max|flow_hours_total_(min|max))'Length of output: 6321
Update parameter names across tests, documentation, and examples for API consistency.
The verification found lingering old parameter names in multiple locations beyond the file in scope:
- Test files (
tests/test_*.py,tests/conftest.py): Still usingon_hours_total_min/max,switch_on_total_max,flow_hours_total_min/max - Documentation examples (
docs/user-guide/mathematical-notation/features/OnOffParameters.md): Example code uses old names - Reference examples (
examples/02_Complex/complex_example.py): All parameter names need updating - Docstring (
flixopt/calculation.py:428): Mentions old names in constraint documentation
While the backward compatibility layer in flixopt/elements.py and flixopt/interface.py keeps these working, the tests and examples should migrate to the new names (on_hours_min/max, switch_on_max, flow_hours_min/max) to serve as proper reference material and ensure consistent migration across the codebase.
🤖 Prompt for AI Agents
In flixopt/components.py around lines 636 to 637, the code and surrounding
project materials still reference old parameter names (on_hours_total_min/max,
switch_on_total_max, flow_hours_total_min/max); update all usages to the new API
names (on_hours_min/on_hours_max, switch_on_max, flow_hours_min/flow_hours_max).
Search and replace these names across tests (tests/test_*.py,
tests/conftest.py), documentation examples
(docs/user-guide/mathematical-notation/features/OnOffParameters.md), reference
examples (examples/02_Complex/complex_example.py), and the docstring in
flixopt/calculation.py (around line 428), ensuring signatures, examples, and
constraint text use the new names; keep the backward-compatibility layer
untouched. After changes, run the test suite and update any failing assertions
or example outputs that reference the old names.
- Fixed weights property normalization to handle scalars/lists properly - Added zero-sum guard to prevent division by zero - Now always aligns weights to model coords before normalizing 2. Documentation - flow_system.py - Added documentation for the weight_of_last_period parameter in the FlowSystem class docstring 3. Code Quality - interface.py - Refactored deprecated kwargs handling to use instance method instead of awkward static call pattern - Removed unnecessary import and cleaner implementation 4. Parameter Name Updates - Test Files Updated deprecated parameter names in all test files: - tests/test_scenarios.py - tests/test_functional.py - tests/conftest.py - tests/test_flow.py - tests/test_linear_converter.py 5. Parameter Name Updates - Documentation Updated parameter names in: - docs/user-guide/mathematical-notation/features/OnOffParameters.md 6. Parameter Name Updates - Examples Updated parameter names in: - examples/02_Complex/complex_example.py 7. Enhanced CHANGELOG.md Added comprehensive migration guidance including: - Clear explanation of weighting behavior for _over_periods constraints - Concrete example showing per-period vs over-periods differences - Removal timeline (version 4.0.0) for deprecated parameters - Simple migration instructions All deprecated parameters: - on_hours_total_min → on_hours_min - on_hours_total_max → on_hours_max - switch_on_total_max → switch_on_max - flow_hours_total_min → flow_hours_min - flow_hours_total_max → flow_hours_max The codebase is now fully updated with consistent naming, proper documentation, and backward compatibility maintained through deprecation warnings!
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/flow_system.py (1)
155-197: Handle single-period edge case in period metadata helpersThe period metadata helpers are well factored, but
FlowSystemcurrently fails when there is only one period andweight_of_last_periodis left asNone:
_create_periods_with_extra()computesperiods[-1] - periods[-2]whenweight_of_last_periodisNone, which raisesIndexErroriflen(periods) == 1.This is an edge case but leads to a hard-to-diagnose error.
Consider guarding this case and raising a clear error (or requiring an explicit weight) when only a single period is provided. For example:
@staticmethod def _create_periods_with_extra(periods: pd.Index, weight_of_last_period: int | float | None) -> pd.Index: - if weight_of_last_period is None: - # Calculate weight from difference between last two periods - weight_of_last_period = int(periods[-1]) - int(periods[-2]) + if weight_of_last_period is None: + if len(periods) < 2: + raise ValueError( + 'FlowSystem: weight_of_last_period must be provided explicitly when only one period is defined.' + ) + # Calculate weight from difference between last two periods + weight_of_last_period = int(periods[-1]) - int(periods[-2])Also applies to: 299-331, 369-401
♻️ Duplicate comments (1)
flixopt/flow_system.py (1)
46-56: Previous review concern aboutweight_of_last_perioddocumentation is addressedThe constructor docstring now documents
weight_of_last_periodalongside the time metadata, with clear semantics and usage, satisfying the earlier request.
🧹 Nitpick comments (5)
flixopt/interface.py (1)
1350-1421: Deprecated property aliases are implemented correctly (minor duplication)The deprecated properties
on_hours_total_min/maxandswitch_on_total_maxcleanly proxy to the new attributes and emitDeprecationWarnings, which is important for backward compatibility with older code and serialized models.If you want to reduce repetition later, you could factor the warning+proxy pattern into a small helper, but it’s not urgent.
CHANGELOG.md (1)
54-107: Changelog entry accurately documents new over-period constraints and renamesThe Unreleased notes clearly explain the new
*_over_periodsparameters, the per-period vs over-period semantics, the Flow/OnOff renames, and theFlowSystemModel.weightsnormalization behavior in line with the implementation.You might optionally add a short note that when
normalize_weights=True, the same normalized weights used in the objective are also used for_over_periodsconstraints, so users know these bounds are interpreted with normalized weights rather than raw period durations.flixopt/elements.py (2)
432-481: New Flow constructor parameters and deprecation handling are well structuredAdding
flow_hours_max,flow_hours_min, and their_over_periodscounterparts, combined with_handle_deprecated_kwarg()and_validate_kwargs(), gives a clean migration path from the old names (flow_hours_total_*,flow_hours_per_period_*,total_flow_hours_*) while preventing silent conflicts when both old and new names are provided.If you expect heavy use of legacy kwargs, you might later centralize the mapping of all old names in a small table for easier auditing, but the current explicit sequence is clear and maintainable.
657-699: Optional refactor: Add explicit dimension validation for improved error clarityThe review comment correctly identifies that
.sum('period')will fail if theperioddimension is missing. xarray raises a ValueError with a clear message when a dimension doesn't exist, so the code will not fail silently. However, the suggested guard adds a custom, user-friendly error message that provides better context specific to FlowSystem configuration.Since the original comment is marked ``, this is an optional enhancement rather than a critical fix. The proposed guard is worth implementing if the project values proactive, domain-specific error messages over relying on xarray's generic dimension errors.
flixopt/effects.py (1)
366-445: Backward-compatible Effect aliases are comprehensive and low-riskThe new alias properties (
minimum_total_per_period/maximum_total_per_period,minimum/maximum) all delegate to the new canonical attributes with deprecation warnings, preserving old user code while keeping a single source of truth for constraints.If you find more legacy names in the wild, consider grouping these aliases in a small mapping table or documenting them in one place to reduce future confusion around which old name maps to which new semantic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.md(1 hunks)docs/user-guide/mathematical-notation/features/OnOffParameters.md(5 hunks)examples/02_Complex/complex_example.py(1 hunks)flixopt/effects.py(13 hunks)flixopt/elements.py(8 hunks)flixopt/flow_system.py(6 hunks)flixopt/interface.py(10 hunks)flixopt/structure.py(1 hunks)tests/conftest.py(1 hunks)tests/test_flow.py(3 hunks)tests/test_functional.py(3 hunks)tests/test_linear_converter.py(2 hunks)tests/test_scenarios.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
examples/02_Complex/complex_example.py (1)
flixopt/interface.py (1)
OnOffParameters(1080-1421)
flixopt/structure.py (2)
flixopt/effects.py (1)
weights(509-527)flixopt/flow_system.py (1)
fit_to_model_coords(606-644)
flixopt/flow_system.py (2)
flixopt/effects.py (1)
weights(509-527)flixopt/structure.py (2)
weights(219-232)hours_of_previous_timesteps(184-185)
flixopt/effects.py (2)
flixopt/structure.py (5)
weights(219-232)add_variables(1274-1283)get_coords(187-216)get_coords(1333-1338)add_constraints(1285-1294)flixopt/flow_system.py (2)
fit_to_model_coords(606-644)coords(1001-1007)
flixopt/elements.py (3)
flixopt/structure.py (4)
_handle_deprecated_kwarg(418-477)_validate_kwargs(479-507)hours_per_step(180-181)hours_per_step(1419-1420)flixopt/flow_system.py (2)
fit_to_model_coords(606-644)coords(1001-1007)flixopt/modeling.py (2)
ModelingPrimitives(193-383)expression_tracking_variable(197-233)
tests/test_functional.py (1)
flixopt/interface.py (1)
OnOffParameters(1080-1421)
flixopt/interface.py (2)
flixopt/structure.py (2)
_handle_deprecated_kwarg(418-477)_validate_kwargs(479-507)flixopt/flow_system.py (1)
fit_to_model_coords(606-644)
tests/test_linear_converter.py (1)
flixopt/interface.py (1)
OnOffParameters(1080-1421)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.13)
🔇 Additional comments (21)
flixopt/interface.py (4)
1106-1137: OnOffParameters arg docs correctly updated to new namesThe docstring now documents
on_hours_min,on_hours_max, andswitch_on_maxand matches the constructor signature and usage in tests/docs. This keeps the mathematical description aligned with the new public API.
1261-1289: Deprecation handling for old OnOffParameters kwargs looks robustUsing
**kwargscombined with_handle_deprecated_kwargand_validate_kwargsgives a clean deprecation path:
- Old names (
on_hours_total_min/max,switch_on_total_max) are mapped to the new ones with clear warnings.- Conflicting specification of old+new raises a
ValueError.- Unknown kwargs are rejected via
_validate_kwargs.This should keep IO and older user code working while enforcing a clear migration route.
1292-1319: transform_data correctly aligns new total-hours and switch limits to period/scenarioMapping
on_hours_max,on_hours_min, andswitch_on_maxthroughfit_to_model_coords(..., dims=['period', 'scenario'])is consistent with “over all periods” semantics:
- Time is not treated as an extra weighting dimension.
- Parameters can be scalar, per-period, per-scenario, or per-(period,scenario) and will broadcast appropriately.
No issues spotted here.
1336-1347: Includingswitch_on_maxinuse_switch_onis necessary for correctnessExtending
use_switch_onto checkself.switch_on_max(in addition toeffects_per_switch_onandforce_switch_on) ensures the model creates switch variables whenever a startup-count limit is set, even without startup effects. This closes a potential gap where a pure-count limit would otherwise be ignored.docs/user-guide/mathematical-notation/features/OnOffParameters.md (1)
237-301: Math docs are consistent with the renamed OnOffParameters APIThe key-parameter list and all examples now use
on_hours_min,on_hours_max, andswitch_on_max, matching the implementation and tests. The formulation for total operating hours and cycling limits remains unchanged, so users get a pure naming migration here.tests/test_linear_converter.py (1)
146-147: Tests correctly migrated toon_hours_min/on_hours_maxBoth on/off-enabled converter tests now construct
OnOffParameterswith the new argument names while still validating theon_hours_totalvariable and constraints in the model. This keeps coverage for the existing behavior and ensures the rename is wired through the modeling layer.Also applies to: 397-398
tests/test_functional.py (1)
481-555: Functional on/off tests aligned with new parameter namesThe on/off functional tests now use
on_hours_min/on_hours_maxinstead of the deprecated_total_variants while preserving their original expectations on costs, on-patterns, and flow rates. This keeps the high-level behavior check intact across the rename.examples/02_Complex/complex_example.py (1)
71-80: Complex example updated to current Flow/OnOffParameters APIThe boiler example now uses
flow_hours_max,on_hours_min/on_hours_max, andswitch_on_max, matching the library’s new parameter names. This keeps the example runnable for new users without relying on deprecated arguments.tests/conftest.py (1)
168-177: Complex boiler fixture matches renamed parametersThe boiler fixture’s On/Off and flow-hours configuration now uses
on_hours_min,on_hours_max,switch_on_max, andflow_hours_max. This keeps all complex tests that depend on this fixture aligned with the updated public API without changing behavior.tests/test_scenarios.py (1)
159-168: Scenario boiler setup now uses new OnOff/Flow parameter namesThe scenario-based boiler in
flow_system_complex_scenariosis updated toon_hours_min,on_hours_max,switch_on_max, andflow_hours_max, matching the rest of the codebase. This keeps scenario tests focused on weights and IO behavior while using the current API.tests/test_flow.py (3)
45-71: Flow hours min/max test now targets new parameter names correctlyUsing
flow_hours_max=1000andflow_hours_min=10and asserting bounds viatotal_flow_hoursmatches the new Flow API and FlowModel bounds; behavior stays equivalent to the old*_total_*names.
974-1029: Startup-count constraint test updated toswitch_on_maxThe test now configures
OnOffParameters(switch_on_max=5)and still checks theswitch|countvariable bounds and constraint consistently with the renamed API.
1036-1065: On-hours limit test aligns withon_hours_min/on_hours_maxsemantics
OnOffParameters(on_hours_min=20, on_hours_max=100)and the corresponding bounds/constraint onon_hours_totalreflect the new naming and the intended per-period on-hours limits.flixopt/flow_system.py (1)
180-197: Propagation of period-weight metadata through serialization looks correctUsing
_compute_period_metadata()in__init__and passingweight_of_last_periodthroughfrom_dataset()ensures period weights are reproducible acrossto_dataset()/from_dataset()cycles while still allowing automatic weight derivation when no explicit weights are given.Also applies to: 519-531
flixopt/elements.py (3)
303-334: Flow API/doc updates for per-period vs over-period hours are consistentThe Flow docstring and examples now clearly differentiate between
flow_hours_min/max(per-period) andflow_hours_min/max_over_periods(weighted across periods), and theswitch_on_maxexample matches the updatedOnOffParametersnaming.Also applies to: 369-383, 410-412
511-538: Transforming new flow-hours bounds to model coordinates matches intended dimensions
flow_hours_max/minare fitted on['period', 'scenario']and the_over_periodsbounds on['scenario'], which aligns with their semantics (per-period vs per-scenario-over-periods constraints) and ensures they broadcast correctly in the FlowModel.
595-635: Deprecatedflow_hours_total_*properties correctly forward to new attributesThe deprecated
flow_hours_total_max/minproperties simply delegate toflow_hours_max/minwith warnings, providing a non-breaking upgrade path while keeping the underlying state single-sourced.flixopt/effects.py (4)
51-71: Effect’s extended API for weights and over-period bounds aligns with the new modeling semanticsIntroducing
weights,minimum_over_periods, andmaximum_over_periodsin the Effect docstring and constructor, together with the examples, matches the intended per-period (minimum_total) vs over-period (*_over_periods) semantics and is consistent with the changelog description.Also applies to: 189-244
485-490: Transforming over-period Effect bounds to scenario coordinates is consistentMapping
minimum_over_periodsandmaximum_over_periodsviafit_to_model_coords(..., dims=['scenario'])fits the idea that these bounds are per-scenario constraints on a total across all periods, independent of the number of periods.
818-826: Objective now correctly uses effect-specific or system weights with explicit normalizationUsing
objective_effect.submodel.weightsand normalizing it whennormalize_weights=Truebefore forming the objective(total * weights).sum() + penaltyis consistent with the new per-effect weighting semantics and avoids relying on implicit normalization in the FlowSystemModel.
508-527: Clarify and decouple normalization behavior for Effect over-period constraintsThe new
EffectModel.weightsproperty and over-period constraint are structurally sound, but there is a confirmed semantic coupling issue:
- Documentation for
minimum_over_periods/maximum_over_periodsstates weights should be "FlowSystem period weights" (implying raw, unnormalized values)- However,
EffectModel.weightsreturnsself._model.weightswhenself.element.weightsisNone, which is already normalized whennormalize_weights=True(seeFlowSystemModel.weightsinstructure.pylines 220-230)- The
minimum_over_periods/maximum_over_periodsconstraint then uses these normalized weights:weighted_total = (self.total * self.weights).sum('period')- Meanwhile,
EffectCollectionModeladditionally renormalizesobjective_weights(lines 821-823), creating different effective scalings for constraints vs. objectives depending on thenormalize_weightsflag- No test coverage validates the intended behavior
To resolve: confirm whether
_over_periodsconstraints should use raw or normalized weights. If raw weights are intended, adjustEffectModel.weightsto return unnormalizedflow_system.weightsand keep normalization confined to the objective layer, similar to the suggested diff in the original comment. This would align implementation with documentation and eliminate the double-normalization issue in the objective.Also applies to: 568-582
…-periods # Conflicts: # CHANGELOG.md # flixopt/effects.py # flixopt/elements.py # flixopt/interface.py
- Fixed weights property normalization to handle scalars/lists properly - Added zero-sum guard to prevent division by zero - Now always aligns weights to model coords before normalizing 2. Documentation - flow_system.py - Added documentation for the weight_of_last_period parameter in the FlowSystem class docstring 3. Code Quality - interface.py - Refactored deprecated kwargs handling to use instance method instead of awkward static call pattern - Removed unnecessary import and cleaner implementation 4. Parameter Name Updates - Test Files Updated deprecated parameter names in all test files: - tests/test_scenarios.py - tests/test_functional.py - tests/conftest.py - tests/test_flow.py - tests/test_linear_converter.py 5. Parameter Name Updates - Documentation Updated parameter names in: - docs/user-guide/mathematical-notation/features/OnOffParameters.md 6. Parameter Name Updates - Examples Updated parameter names in: - examples/02_Complex/complex_example.py 7. Enhanced CHANGELOG.md Added comprehensive migration guidance including: - Clear explanation of weighting behavior for _over_periods constraints - Concrete example showing per-period vs over-periods differences - Removal timeline (version 4.0.0) for deprecated parameters - Simple migration instructions All deprecated parameters: - on_hours_total_min → on_hours_min - on_hours_total_max → on_hours_max - switch_on_total_max → switch_on_max - flow_hours_total_min → flow_hours_min - flow_hours_total_max → flow_hours_max The codebase is now fully updated with consistent naming, proper documentation, and backward compatibility maintained through deprecation warnings!
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
flixopt/flow_system.py (1)
155-189: Missingweight_per_periodattribute causes runtime error in FlowModel
FlowSystem.__init__currently discards the computedweight_per_periodinstead of storing it, butFlowModelrelies onself._model.flow_system.weight_per_periodwhen building the newflow_hours_*_over_periodsconstraints. This will raise anAttributeErrorat modeling time.You can fix this by assigning
weight_per_periodto an instance attribute and using it when defaultingweights:- # Compute all period-related metadata using shared helper - (self.periods_extra, self.weight_of_last_period, weight_per_period) = self._compute_period_metadata( - self.periods, weight_of_last_period - ) + # Compute all period-related metadata using shared helper + ( + self.periods_extra, + self.weight_of_last_period, + self.weight_per_period, + ) = self._compute_period_metadata(self.periods, weight_of_last_period) @@ - self.hours_per_timestep = self.fit_to_model_coords('hours_per_timestep', hours_per_timestep) - - self.weights = self.fit_to_model_coords( - 'weights', weights if weights is not None else weight_per_period, dims=['period', 'scenario'] - ) + self.hours_per_timestep = self.fit_to_model_coords('hours_per_timestep', hours_per_timestep) + + # Use user-provided weights when available; otherwise derive from per-period weights + self.weights = self.fit_to_model_coords( + 'weights', + weights if weights is not None else self.weight_per_period, + dims=['period', 'scenario'], + )CHANGELOG.md (1)
52-135: Minor wording/typo fixes in Unreleased changelogThe Unreleased notes accurately describe the new over-period constraints and parameter renames, but there are a couple of small textual issues you might want to clean up:
- “emmit warnings” → “emit warnings”.
- “HetaPump” / “HetaPumpWithSource” → “HeatPump” / “HeatPumpWithSource” in the efficiency-parameter bullet list.
These are purely cosmetic but will make the changelog read more polished.
🧹 Nitpick comments (1)
flixopt/elements.py (1)
677-724: Over-periods constraint logic is sound but may need a no-scenario fallbackThe new
flow_hours_*_over_periodslogic correctly:
- derives a weighted or unweighted sum over the period dimension, and
- tracks it via
flow_hours_over_periodswith bounds fromflow_hours_min/max_over_periods.One edge case to double-check is deterministic (no-scenario) models with periods defined: both
transform_data()and the tracking variable here usecoords=['scenario']/ dims['scenario']. Ifflow_system.scenariosisNone,model.get_coords(['scenario'])may not behave as expected.Consider guarding the coords argument, e.g.:
coords = ['scenario'] if self._model.flow_system.scenarios is not None else None ... ModelingPrimitives.expression_tracking_variable( model=self, ..., coords=coords, )and letting
_fit_coordsfor the_over_periodsbounds follow the same pattern, so single-scenario (or no-scenario) period models are handled cleanly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.md(3 hunks)examples/02_Complex/complex_example.py(1 hunks)flixopt/components.py(1 hunks)flixopt/effects.py(13 hunks)flixopt/elements.py(8 hunks)flixopt/features.py(2 hunks)flixopt/flow_system.py(6 hunks)flixopt/interface.py(10 hunks)flixopt/linear_converters.py(1 hunks)flixopt/structure.py(0 hunks)tests/conftest.py(1 hunks)tests/test_functional.py(3 hunks)tests/test_scenarios.py(1 hunks)
💤 Files with no reviewable changes (1)
- flixopt/structure.py
✅ Files skipped from review due to trivial changes (2)
- flixopt/components.py
- flixopt/features.py
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/02_Complex/complex_example.py
- flixopt/effects.py
- tests/test_functional.py
- tests/conftest.py
🧰 Additional context used
🧬 Code graph analysis (3)
flixopt/elements.py (3)
flixopt/structure.py (4)
_handle_deprecated_kwarg(479-538)_validate_kwargs(540-568)_fit_coords(311-324)flow_system(291-309)flixopt/modeling.py (2)
ModelingPrimitives(193-383)expression_tracking_variable(197-233)flixopt/flow_system.py (1)
coords(1062-1068)
flixopt/interface.py (1)
flixopt/structure.py (3)
_handle_deprecated_kwarg(479-538)_validate_kwargs(540-568)_fit_coords(311-324)
flixopt/flow_system.py (2)
flixopt/effects.py (1)
weights(467-483)flixopt/structure.py (1)
hours_of_previous_timesteps(186-187)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (6)
flixopt/interface.py (1)
1128-1448: OnOffParameters deprecation mapping and new fields look consistentThe ctor’s handling of
on_hours_total_min/maxandswitch_on_total_maxvia_handle_deprecated_kwarg, plus the newon_hours_min/maxandswitch_on_maxattributes and transform_data alignment to['period', 'scenario'], is consistent with the existing deprecation pattern and keepsuse_switch_onwired to the new field correctly. The deprecated proxy properties also provide a clean, backwards‑compatible path.flixopt/linear_converters.py (1)
680-692: CHP example now matches renamed OnOffParameters APIThe CHP example’s switch from
on_hours_total_maxtoon_hours_maxmatches the new OnOffParameters signature and avoids relying on deprecated aliases.tests/test_scenarios.py (1)
140-168: Scenario tests correctly updated to new parameter namesUpdating the boiler Flow to
flow_hours_maxand the nested OnOffParameters toon_hours_min/on_hours_max/switch_on_maxkeeps the tests aligned with the new API while preserving their intent.flixopt/elements.py (3)
446-505: Flow ctor deprecations and new over-period parameters are wired correctlyThe Flow initializer’s handling of deprecated kwarg names (
flow_hours_per_period_*,flow_hours_total_*,total_flow_hours_*) via_handle_deprecated_kwarg, followed by_validate_kwargs, cleanly maps callers onto the newflow_hours_max/minandflow_hours_max/min_over_periodsattributes without leaving stray kwargs. The new attributes are then used consistently intransform_data()and FlowModel (per-period tracking bounds and over-period constraints), matching the semantics described in the docstring.
533-550: transform_data dimension choices for new Flow hour constraintsMapping
flow_hours_max/minto['period', 'scenario']and the_over_periodsbounds to['scenario']fits the intended semantics (per-period vs across-periods per scenario). This should give xarray enough information to align/broadcast against model coordinates once period weights are available via FlowSystem.
613-652: Deprecated Flowflow_hours_total_*properties provide smooth migrationThe deprecated
flow_hours_total_max/minproperties correctly proxy toflow_hours_max/minwith clear DeprecationWarnings. This matches the changelog’s migration guidance and keeps older user code functioning while encouraging the new names.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/effects.py (1)
181-244: Add validation guard forminimum_over_periods/maximum_over_periodsrequiring a period dimension.Your concern is valid. The code at line 531 calls
.sum('period')without checking whether the'period'dimension exists:weighted_total = (self.total * self.weights).sum('period')If a user defines
minimum_over_periodsormaximum_over_periodson aFlowSystemwith scenarios but no periods,transform_data()will successfully fit these parameters on['scenario'](line 442–449), butEffectModel._do_modeling()will fail at runtime with an opaque xarray error when attempting the.sum('period')operation.The
_plausibility_checks()method (lines 455–458) is currently empty (onlypass), so no early validation exists.Recommended fixes:
- Add explicit documentation stating that
minimum_over_periodsandmaximum_over_periodsrequire a'period'dimension.- Add a guard in
_plausibility_checks()or at the start of_do_modeling()that raises a clearValueErrorif these parameters are set but'period'is absent from the model's coordinates.This ensures users receive a clear configuration error rather than a runtime xarray exception.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
flixopt/effects.py(13 hunks)flixopt/structure.py(1 hunks)tests/test_scenarios.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
flixopt/structure.py (1)
flixopt/effects.py (3)
weights(467-484)objective_effect(694-697)objective_effect(700-703)
tests/test_scenarios.py (3)
flixopt/structure.py (6)
objective_weights(190-194)values(1266-1271)values(1553-1554)variables(1456-1462)to_dataset(734-757)from_dataset(778-811)flixopt/effects.py (1)
weights(467-484)flixopt/flow_system.py (5)
FlowSystem(38-1493)connect_and_transform(667-682)to_dataset(482-494)from_dataset(497-553)sel(1202-1230)
flixopt/effects.py (2)
flixopt/structure.py (6)
_fit_coords(318-331)add_variables(1343-1352)get_coords(196-225)get_coords(1402-1407)add_constraints(1354-1363)objective_weights(190-194)flixopt/flow_system.py (1)
coords(1062-1068)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (5)
tests/test_scenarios.py (4)
159-168: On/Off parameter rename usage looks consistent.The switch to
on_hours_min/on_hours_max/switch_on_maxand addedflow_hours_maxmatches the new API and keeps semantics clear; no issues spotted in this setup.
244-252: Objective weighting tests correctly targetmodel.objective_weights.The updated checks compare
model.objective_weightsto a manually normalized version ofFlowSystem.weightsand verify the objective expression uses those weights, with.sum().item()avoiding anyxarray.DataArraytruth‑value issues. This is a good alignment with the newobjective_weightsAPI; just ensurecreate_linopy_modelkeepsnormalize_weightsbehavior consistent with these expectations.
260-262: IO test for normalized weights is aligned withobjective_weightssemantics.Here you pass already normalized weights and assert
model.objective_weightsmatches them and sums to 1.0, which nicely complements the previous test that exercises in‑model normalization from raw weights. The logic looks sound.
695-771: New weights IO/selection tests accurately exercise FlowSystem behavior.
test_weights_io_persistenceverifies thatFlowSystem.to_dataset()/from_dataset()round‑trip preserves both values and dimensions ofweights, which is important now that weights are part of the core objective machinery.test_weights_selectionconfirms.sel(scenario=...)slicesweightsconsistently with the scenario index and keeps it strictly 1D on thescenariodim, matching the intended API.Both tests look correct and provide valuable coverage for the new weights plumbing.
flixopt/effects.py (1)
786-790: Objective construction now cleanly usesmodel.objective_weights.Switching the objective to:
self._model.add_objective( (self.effects.objective_effect.submodel.total * self._model.objective_weights).sum() + self.penalty.total.sum() )nicely centralizes weight selection/normalization in
FlowSystemModel.objective_weightsand aligns test expectations (model.objective_weightsintests/test_scenarios.py). Dimensionally this is consistent:totalover (period, scenario) combined with scenario/period weights, plus scalar penalty.This change looks correct and improves encapsulation of weighting semantics.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
flixopt/effects.py (2)
445-452: Over-periods bounds + plausibility check form a coherent, guarded feature.The addition of
minimum_over_periods/maximum_over_periods, their fitting toscenariocoords, the PlausibilityError when periods are absent, and the weightedtotal_over_periodsconstraint together give a clear and robust implementation of “sum over all periods” bounds while preventing the earliersum('period')on a period-less system.Also applies to: 459-467, 538-551
476-495:EffectModel.weightsdocstring still overstates dimensionality in period-less setups.The implementation is solid: it prefers effect-specific weights, then
FlowSystem.weights, and finally falls back to_fit_coords(..., data=1, dims=['period', 'scenario']), which is a nice improvement over the previous scalarxr.DataArray(1). However, the docstring’s “Weights for period and scenario dimensions” isn’t strictly true when the model has noperiodcoord (in that case weights will typically be('scenario',)only). Consider rephrasing to something like “weights aligned to the effect’s total over the available period/scenario dimensions” to avoid confusion.
🧹 Nitpick comments (1)
flixopt/effects.py (1)
192-193: Effect-specific weights plumbing looks correct; minor dim-order nit.
weightsis threaded from the constructor intotransform_dataand aligned via_fit_coords(..., dims=['scenario', 'period']), and then consumed throughEffectModel.weights, which is consistent with how model coords are handled elsewhere. Since other weights in the system (e.g.,FlowSystem.weights,EffectModel.weightsfallback) use['period', 'scenario'], you might consider using the same dim order here for readability/consistency, though xarray will align by name either way.Also applies to: 212-212, 451-452
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
flixopt/effects.py(13 hunks)flixopt/structure.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- flixopt/structure.py
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/effects.py (3)
flixopt/core.py (1)
PlausibilityError(21-24)flixopt/structure.py (4)
FlowSystemModel(81-249)flow_system(302-320)_fit_coords(322-335)objective_weights(190-198)flixopt/flow_system.py (2)
FlowSystem(38-1493)coords(1062-1068)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (2)
flixopt/effects.py (2)
369-407: New deprecated total-per-period properties mirror existing patterns.The
minimum_total_per_period/maximum_total_per_periodproperties follow the same warning + forwarding pattern as the other deprecated aliases, which should preserve backward compatibility cleanly.
796-799: Objective now correctly uses normalized objective weights.Switching the objective to
(objective_effect.total * self._model.objective_weights).sum() + self.penalty.total.sum()cleanly centralizes weighting/normalization inFlowSystemModel.objective_weightsand respects effect-specific weights, which is a nice improvement over using raw system weights directly.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_scenarios.py (1)
24-31: Prefer scenario_weights keyword over legacy weights in FlowSystem test fixtureIn the
test_systemfixture, the FlowSystem is still constructed with:flow_system = FlowSystem( timesteps=timesteps, scenarios=scenarios, weights=weights, )Given the public API rename to
scenario_weights, it would be better to exercise the new keyword directly (and avoid depending on any deprecated alias handling), e.g.:- flow_system = FlowSystem( - timesteps=timesteps, - scenarios=scenarios, - weights=weights, - ) + flow_system = FlowSystem( + timesteps=timesteps, + scenarios=scenarios, + scenario_weights=weights, + )This keeps the tests aligned with the current constructor and makes failures more informative if the old name is ever removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
examples/04_Scenarios/scenario_example.py(1 hunks)flixopt/effects.py(13 hunks)flixopt/elements.py(8 hunks)flixopt/flow_system.py(10 hunks)flixopt/plotting.py(1 hunks)flixopt/results.py(1 hunks)flixopt/structure.py(1 hunks)tests/conftest.py(2 hunks)tests/test_scenarios.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/conftest.py
🧰 Additional context used
🧬 Code graph analysis (6)
flixopt/structure.py (2)
flixopt/flow_system.py (1)
coords(1116-1122)flixopt/effects.py (3)
period_weights(479-496)objective_effect(706-709)objective_effect(712-715)
examples/04_Scenarios/scenario_example.py (2)
flixopt/structure.py (2)
flow_system(322-340)scenario_weights(190-208)flixopt/flow_system.py (1)
FlowSystem(38-1557)
tests/test_scenarios.py (3)
flixopt/structure.py (5)
scenario_weights(190-208)objective_weights(211-218)variables(1480-1486)to_dataset(758-781)from_dataset(802-835)tests/conftest.py (1)
create_linopy_model(721-733)flixopt/flow_system.py (5)
FlowSystem(38-1557)connect_and_transform(723-736)to_dataset(538-550)from_dataset(553-609)sel(1261-1289)
flixopt/elements.py (3)
flixopt/structure.py (4)
_handle_deprecated_kwarg(510-569)_validate_kwargs(571-599)_fit_coords(342-355)flow_system(322-340)flixopt/modeling.py (2)
ModelingPrimitives(193-383)expression_tracking_variable(197-233)flixopt/flow_system.py (1)
coords(1116-1122)
flixopt/flow_system.py (2)
flixopt/structure.py (2)
scenario_weights(190-208)_resolve_dataarray_reference(629-661)flixopt/effects.py (1)
period_weights(479-496)
flixopt/effects.py (4)
flixopt/core.py (1)
PlausibilityError(21-24)flixopt/features.py (1)
ShareAllocationModel(530-633)flixopt/structure.py (9)
FlowSystemModel(81-269)Submodel(1343-1522)flow_system(322-340)_fit_coords(342-355)add_variables(1367-1376)get_coords(220-249)get_coords(1426-1431)add_constraints(1378-1387)objective_weights(211-218)flixopt/flow_system.py (2)
FlowSystem(38-1557)coords(1116-1122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
🔇 Additional comments (15)
flixopt/results.py (1)
1231-1235: Facet example doc tweak is consistent with new period/scenario orderingThe updated example for
facet_by=['period', 'scenario']matches the new dimension semantics and is clear.examples/04_Scenarios/scenario_example.py (1)
82-88: FlowSystem constructor now correctly uses scenario_weightsSwitching to
scenario_weights=scenario_weightsaligns this example with the updated FlowSystem API and keeps the probabilities explicit and normalized.tests/test_scenarios.py (4)
140-168: OnOffParameters and Flow argument renames are used correctly in complex scenario fixtureThe updated arguments:
on_hours_min/on_hours_maxswitch_on_maxflow_hours_maxmatch the renamed API on
OnOffParametersandFlowand keep the previous intent (very loose bounds for this stress test). No behavioral issues spotted here.
238-267: Weighting tests correctly cover normalized vs pre-normalized scenario weights
test_weightsandtest_weights_ionow:
- Drive weighting via
flow_system_piecewise_conversion_scenarios.scenario_weights.- Use
flow_system._compute_weights()andflow_system.weightsonly to build expectations.- Assert
model.objective_weightsmatches the normalized product used in the objective expression, and that it sums to 1.This exercises both cases (raw scenario weights and already-normalized weights) against
FlowSystemModel.objective_weights, which is exactly what you want after introducing scenario-based weighting. The reliance on the private_compute_weights()is acceptable in tests given the tight coupling to the model’s internal semantics.
321-343: Scenario subset test keeps objective and weights semantics in sync when normalize_weights=FalseIn
test_scenarios_selection, you:
- Assign explicit
scenario_weightsand deriveweightsvia_compute_weights().- Slice the FlowSystem with
.sel(scenario=scenarios[0:2]).- Assert that
flow_system.weightsmatches the sliced original weights and use those directly in the objective check withnormalize_weights=False.This is a good regression test for keeping FlowSystem-level weights and the model’s objective consistent under scenario sub-selection and disabled normalization.
700-775: New weights IO and selection tests solidify the scenario-weighting contractThe new tests:
test_weights_io_persistence: round-trips a FlowSystem with customscenario_weightsthroughto_dataset()/from_dataset()and checks thatfs_loaded.weightsmatchesfs_original.weights(including dims).test_weights_selection: verifies thatFlowSystem.sel(scenario=[...])slicesweightsconsistently with scenario selection and that the resulting weights remain 1D overscenario.These are valuable high-level guards around the new weighting plumbing (serialization and selection). Once the underlying
FlowSystemModel.scenario_weightsbug is fixed, these should give good confidence that end-to-end behavior remains stable across IO and subsetting.flixopt/elements.py (3)
475-493: LGTM! Clean deprecation handling for parameter renames.The multi-stage deprecation chain (flow_hours_per_period_max → flow_hours_total_max → flow_hours_max) correctly handles both the intermediate and older parameter names, allowing gradual migration. The use of centralized helpers from structure.py ensures consistent behavior across the codebase.
539-550: Correct dimension mapping for per-period vs over-period constraints.The dimension assignments properly distinguish between per-period constraints (dims=['period', 'scenario']) and across-period constraints (dims=['scenario']), matching the intended semantics where flow_hours_max/min apply per period and flow_hours_max/min_over_periods apply to the weighted sum across all periods.
692-721: Well-implemented weighted over-periods constraint with proper validation.The constraint correctly:
- Validates period dimension existence before attempting the sum (lines 695-699)
- Uses FlowSystem.period_weights to compute the weighted sum (line 701)
- Creates a tracking variable with appropriate bounds and coordinates (lines 706-720)
The dependency on FlowSystem.period_weights is safe because line 695 ensures periods are defined, and period_weights is always computed when periods exist (per flow_system.py lines 186-192).
flixopt/flow_system.py (3)
293-329: Well-structured period metadata computation mirroring time handling.The static methods
_create_periods_with_extraandcalculate_weight_per_periodcorrectly mirror the time metadata pattern:
- Infer weight_of_last_period from differences when not provided (lines 304-310)
- Require explicit weight when only one period exists (lines 305-308)
- Compute per-period weights from index differences (line 327)
- Return properly dimensioned DataArray (line 328)
This parallel structure ensures consistency across time and period dimensions.
367-399: Clean orchestration of period metadata computation.The classmethod correctly:
- Returns None for all outputs when periods are not defined (lines 385-386)
- Delegates to static helpers for actual computation (lines 389, 392)
- Extracts weight_of_last_period from computed weights when not provided (lines 395-396)
- Maintains consistency with the time metadata pattern
444-497: Correct period metadata updates for dataset operations.The method properly:
- Recomputes period metadata from the new period index (lines 469-471)
- Updates period_weights DataArray if present (lines 473-475)
- Maintains backward compatibility by recomputing the composite 'weights' variable (lines 477-490)
The composition logic at lines 482-488 correctly handles both cases: using just period_weights when scenario_weights is absent, or multiplying them when both exist.
flixopt/effects.py (3)
460-469: Excellent validation of dimension requirements for over-periods constraints.The plausibility check correctly prevents configuration errors by:
- Validating that periods are defined when over-periods constraints are used (lines 462-464)
- Providing a clear, actionable error message (lines 465-469)
- Raising PlausibilityError at the appropriate time (before modeling)
This mirrors the validation pattern in elements.py FlowModel (lines 695-699) for consistency.
478-497: Well-designed three-tier fallback for period weights.The property correctly implements cascading fallback:
- Effect-specific period_weights take precedence (lines 492-493)
- FlowSystem period_weights serve as default (lines 494-495)
- Fitted scalar (line 496) ensures proper dimensions when neither exists
Line 496 uses
_fit_coordsrather than a bare scalar, ensuring the fallback value has correct period dimensions when broadcast. This addresses the dimensional consistency concern from earlier reviews.
798-802: Approved:objective_weightsproperty correctly implements combined weights.The verification confirms that
objective_weightsexists as a property on FlowSystemModel (flixopt/structure.py:211) and properly combines period and scenario weights for the optimization objective. Line 800 correctly uses this property.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
flixopt/flow_system.py (1)
35-36: Scenario weights plumbing is solid; consider tightening deprecation and validation behaviorThe new wiring for
scenario_weights(constructor arg, property backed byfit_to_model_coords, and deprecatedweightsproxy) looks coherent and matches the model-side API (FlowSystemModel.scenario_weights/objective_weights). A couple of small behavioral tweaks would make it more robust:
- When both
weightsandscenario_weightsare passed to__init__,scenario_weightsis silently overridden by the deprecatedweightsbecause_handle_deprecated_kwarg(..., check_conflict=False, current_value=scenario_weights)always returns the old value when present. That’s surprising for users migrating to the new name. Consider enabling conflict detection here so dual usage is rejected explicitly, or at least preferringscenario_weightswhen both are given:- scenario_weights = self._handle_deprecated_kwarg( - kwargs, - 'weights', - 'scenario_weights', - scenario_weights, - check_conflict=False, - additional_warning_message='This might lead to later errors if your custom weights used the period dimension.', - ) + scenario_weights = self._handle_deprecated_kwarg( + kwargs, + 'weights', + 'scenario_weights', + scenario_weights, + # New name should win if both are provided; let this raise in that case + check_conflict=True, + additional_warning_message='This might lead to later errors if your custom weights used the period dimension.', + )
scenario_weightssetter happily accepts non-scalar values even ifself.scenarios is None, which would later break atFlowSystemModel.scenario_weights(it always sums over the'scenario'dim). You might want to guard that case to fail early:@scenario_weights.setter - def scenario_weights(self, value: Numeric_S) -> None: + def scenario_weights(self, value: Numeric_S | None) -> None: """ Set scenario weights. @@ - self._scenario_weights = self.fit_to_model_coords('scenario_weights', value, dims=['scenario']) + if value is None: + self._scenario_weights = None + return + + if self.scenarios is None: + raise ValueError('FlowSystem.scenario_weights cannot be set when no scenarios are defined.') + + self._scenario_weights = self.fit_to_model_coords('scenario_weights', value, dims=['scenario'])These changes would keep the public API semantics clear while avoiding subtle surprises around deprecation and invalid configurations.
Also applies to: 54-59, 156-177, 194-201, 1123-1166
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
flixopt/flow_system.py(11 hunks)tests/test_scenarios.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
flixopt/flow_system.py (2)
flixopt/structure.py (6)
scenario_weights(190-213)_handle_deprecated_kwarg(515-579)_validate_kwargs(581-609)get(1429-1434)get(1594-1596)_resolve_dataarray_reference(639-671)flixopt/effects.py (1)
period_weights(479-496)
tests/test_scenarios.py (3)
flixopt/flow_system.py (10)
scenario_weights(1124-1131)scenario_weights(1134-1141)FlowSystem(38-1596)coords(1111-1117)connect_and_transform(718-731)to_dataset(533-545)from_dataset(548-604)weights(1144-1150)weights(1153-1165)sel(1300-1328)flixopt/structure.py (6)
scenario_weights(190-213)flow_system(327-345)objective_weights(216-223)variables(1490-1496)to_dataset(768-791)from_dataset(812-845)tests/conftest.py (1)
create_linopy_model(721-733)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (3)
tests/test_scenarios.py (3)
4-4: Fixture updates align with new scenario and on/off parameter APIs
- Importing
xarray as xrand usingscenario_weights(as a NumPy array) in thetest_systemfixture are consistent withFlowSystem.__init__(..., scenario_weights=...)and the newscenario_weightsplumbing.- The renamed
OnOffParametersarguments (on_hours_min,on_hours_max,switch_on_max,flow_hours_max) match the updated public API and keep this complex fixture representative of real usage.No issues from a test-behavior perspective here.
Also applies to: 24-32, 160-169
239-277: Good coverage of normalized scenario weighting in the model objective
test_weightsandtest_weights_ionicely validate the new semantics:
- They set
flow_system_piecewise_conversion_scenarios.scenario_weights(as both raw NumPy array andxr.DataArray) and assert thatmodel.objective_weightsequals the normalized scenario weights, matchingFlowSystemModel.scenario_weightsbehavior withnormalize_weights=True.- The use of
assert_linequalonmodel.objective.expressionverifies that the objective correctly multiplies thecostsvariable by the normalized weights and adds thePenaltyterm.These tests should catch most regressions in scenario-weight normalization and objective construction.
331-351: Weights IO and selection tests correctly exercise the deprecated proxy and new scenario_weights behaviorThe new and updated tests around weights persistence and slicing look well designed:
test_scenarios_selectionnow setsflow_system_full.scenario_weightsand still asserts viaflow_system.weights, validating that the deprecatedweightsproperty continues to proxy toscenario_weightsand thatsel(scenario=...)correctly slices the underlying weights.test_weights_io_persistencechecks that customscenario_weightssurvive ato_dataset/from_datasetround-trip and that dims are preserved, which directly exercises the new FlowSystem serialization logic for scenario weights.test_weights_selectionconfirms thatFlowSystem.sel()slicesweights/scenario_weightsconsistently with scenario selection and that the result remains a pure 1D('scenario',)array.Together with the existing full-calculation IO tests, this gives solid coverage of the new weighting semantics and the deprecation shim.
Also applies to: 709-783
| # Compute all period-related metadata using shared helper | ||
| (self.periods_extra, self.weight_of_last_period, weight_per_period) = self._compute_period_metadata( | ||
| self.periods, weight_of_last_period | ||
| ) | ||
|
|
||
| self.period_weights: xr.DataArray | None = weight_per_period | ||
|
|
There was a problem hiding this comment.
Single-period selections will currently crash due to period metadata recomputation
The new period metadata helpers are a good mirror of the existing time metadata pattern, but there is a problematic edge case in _update_period_metadata:
_create_periods_with_extraexplicitly requires an explicitweight_of_last_periodwhenlen(periods) < 2and raises aValueErrorotherwise, which makes sense during initial construction (no interval to infer from)._update_period_metadataalways calls_compute_period_metadata(new_period_index, weight_of_last_period)withweight_of_last_period=Nonefrom_dataset_sel/_dataset_isel, whenever'period'is in the indexers andlen(new_period_index) >= 1.- For a selection/isel that leaves exactly one period (a realistic use case), this combination means
_create_periods_with_extrawill always raise, and there is currently no way for callers ofsel/iselto provideweight_of_last_periodexplicitly.
So FlowSystem.sel(period=single_label) or FlowSystem._dataset_sel(..., period=...) will blow up for single-period subsets, even though the original dataset already contains a valid weight_of_last_period in its attrs.
A minimal fix is to reuse the existing attribute value when no explicit override is provided, so that single-period subsets can still compute period_weights:
@classmethod
def _update_period_metadata(
cls,
dataset: xr.Dataset,
weight_of_last_period: int | float | None = None,
) -> xr.Dataset:
@@
- new_period_index = dataset.indexes.get('period')
- if new_period_index is not None and len(new_period_index) >= 1:
- # Use shared helper to compute all period metadata
- _, weight_of_last_period, period_weights = cls._compute_period_metadata(
- new_period_index, weight_of_last_period
- )
+ new_period_index = dataset.indexes.get('period')
+ if new_period_index is not None and len(new_period_index) >= 1:
+ # Reuse stored weight_of_last_period when not explicitly overridden;
+ # this is essential for single-period subsets where it cannot be inferred.
+ if weight_of_last_period is None:
+ weight_of_last_period = dataset.attrs.get('weight_of_last_period')
+
+ # Use shared helper to compute all period metadata
+ _, weight_of_last_period, period_weights = cls._compute_period_metadata(
+ new_period_index, weight_of_last_period
+ )With this change:
- Multi-period selections still recompute
period_weightsconsistently (since the storedweight_of_last_periodshould equal the last interval anyway). - Single-period selections can rely on the preserved
weight_of_last_periodfrom the original system instead of crashing. - The new calls from
_dataset_sel/_dataset_iselwill behave correctly for all subset sizes.
Also, minor doc nit: the _update_period_metadata docstring still says “recomputes … period_weights, and weights” even though there’s no dataset-level weights anymore; consider updating that wording for clarity.
Also applies to: 303-339, 377-409, 454-491, 1293-1297, 1374-1377
🤖 Prompt for AI Agents
In flixopt/flow_system.py around lines 196 to 202, _update_period_metadata
currently calls _compute_period_metadata with weight_of_last_period=None for
selections that leave exactly one period, which triggers
_create_periods_with_extra to raise; fix by falling back to the FlowSystem's
existing stored attr (self.weight_of_last_period or dataset attrs) when
weight_of_last_period is None so single-period sel/isel reuse the original
weight_of_last_period; update the logic to pass that fallback value into
_compute_period_metadata and adjust the docstring wording to remove reference to
non-existent dataset-level "weights".
Description
Allow to specify min max sums over all periods
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
Changed
Deprecated
Bug Fixes
Documentation