Feature/excess rename #501
Conversation
|-------------------------|----------------------------------------------------------------------------------|
| flixopt/elements.py | Renamed attributes excess_input → virtual_supply, excess_output → virtual_demand |
| flixopt/optimization.py | Updated attribute access and result keys |
| tests/test_bus.py | Updated variable name strings in assertions |
| docs/.../Bus.md | Updated description of φ symbols |
The variable names in the optimization model are now:
- {BusName}|virtual_supply (was excess_input)
- {BusName}|virtual_demand (was excess_output)
|
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 You can disable this status message by setting the WalkthroughThis PR implements large-scale public API refactoring involving parameter and attribute renames (excess → imbalance semantics, with new virtual supply/demand concepts), removal of deprecated calculation modules with migration pathways, and updates to related method signatures and results structures across the codebase. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
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 |
# Conflicts: # CHANGELOG.md # flixopt/flow_system.py
|
@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: 0
🧹 Nitpick comments (5)
docs/user-guide/mathematical-notation/elements/Bus.md (1)
8-9: Fix pre-existing typos in documentation.While reviewing the updated documentation, I noticed some pre-existing typos that should be corrected:
- Line 8: "a
imbalance_penalty_per_flow_hour" → "animbalance_penalty_per_flow_hour"- Line 9: "usefull" → "useful"
- Line 9: "ifeasiblity" → "infeasibility"
-Optionally, a Bus can have a `imbalance_penalty_per_flow_hour` parameter, which allows to penaltize the balance for missing or excess flow-rates. -This is usefull as it handles a possible ifeasiblity gently. +Optionally, a Bus can have an `imbalance_penalty_per_flow_hour` parameter, which allows to penalize the balance for missing or excess flow-rates. +This is useful as it handles a possible infeasibility gently.tests/test_bus.py (2)
39-52: Virtual supply/demand test expectations align with BusModelThe expectations for:
- variables:
TestBus|virtual_supply,TestBus|virtual_demand, andbalanceconstraint:GastarifTest(Q_Gas)|flow_rate - WärmelastTest(Q_th_Last)|flow_rate + virtual_supply - virtual_demand == 0are consistent with the BusModel implementation (
Σ(inflows) + virtual_supply = Σ(outflows) + virtual_demand). Theassert_var_equalchecks againstmodel.add_variables(lower=0, coords=...)are a bit indirect but correctly validate bounds and coordinates.If you want the tests to be less side‑effecty, consider exposing a small helper that creates an anonymous variable with the same defaults instead of calling
add_variablesinside the assertion.Also applies to: 54-60
68-80: Penalty share test matches new imbalance semantics; comments mention “excess”The final penalty constraint check correctly asserts that
TestBus->Penalty(temporal)equals(virtual_supply + virtual_demand) * 1e5 * hours_per_step, which matches the BusModel’s use ofimbalance_penalty_per_flow_hour. The only nit is that the comments still refer to “excess” rather than imbalance/virtual supply/demand, which might confuse readers.You could update Lines 68–70 to use the new terminology (imbalance, virtual_supply/virtual_demand) to keep tests and docs aligned.
flixopt/elements.py (2)
246-259: Bus constructor, data transform, and plausibility checks look coherent
- The
__init__signature withimbalance_penalty_per_flow_hourand**kwargs, plus_handle_deprecated_kwarg(...)forexcess_penalty_per_flow_hour, provides a clean migration path while still validating unexpected kwargs.transform_datacorrectly routesimbalance_penalty_per_flow_hourthrough_fit_coords, consistent with other TPS‑style parameters._plausibility_checksreuses the existing pattern to warn on all‑zero penalties and still enforces the “at least one flow” invariant.allows_imbalanceasimbalance_penalty_per_flow_hour is not Noneis simple and used appropriately in BusModel.No functional issues spotted here.
If you want to be stricter, you could later extend
_plausibility_checksto reject negative penalties rather than silently allowing them.Also applies to: 273-277, 279-286, 291-293
863-891: BusModel balance and penalty logic are mathematically sound; minor clarity improvements possibleThe BusModel changes behave as intended:
virtual_supply/virtual_demandare only created whenallows_imbalanceis true.- The modified balance constraint effectively enforces
Σ(inflows) + virtual_supply = Σ(outflows) + virtual_demand,
which matches the docstring and tests (they check theinputs - outputs + virtual_supply - virtual_demand == 0form).- The penalty scaling
imbalance_penalty = hours_per_step * imbalance_penalty_per_flow_hourmirrors the FlowModel shares pattern, and separate temporal penalty shares for supply and demand are consistent with the tests.Two small readability/maintenance nits:
- Line 890’s
eq_bus_balance.lhs -= -self.virtual_supply + self.virtual_demandis algebraically correct but a bit opaque.eq_bus_balance.lhs += self.virtual_supply - self.virtual_demandwould be easier to read.- The two
add_share_to_effectscalls could be combined into a single call with a summed expression to make it obvious that the total penalty is(virtual_supply + virtual_demand) * imbalance_penalty.You could refactor this block as:
- if self.element.allows_imbalance: - imbalance_penalty = np.multiply(self._model.hours_per_step, self.element.imbalance_penalty_per_flow_hour) - - self.virtual_supply = self.add_variables( - lower=0, coords=self._model.get_coords(), short_name='virtual_supply' - ) - - self.virtual_demand = self.add_variables( - lower=0, coords=self._model.get_coords(), short_name='virtual_demand' - ) - - eq_bus_balance.lhs -= -self.virtual_supply + self.virtual_demand + if self.element.allows_imbalance: + imbalance_penalty = np.multiply( + self._model.hours_per_step, self.element.imbalance_penalty_per_flow_hour + ) + + self.virtual_supply = self.add_variables( + lower=0, coords=self._model.get_coords(), short_name='virtual_supply' + ) + self.virtual_demand = self.add_variables( + lower=0, coords=self._model.get_coords(), short_name='virtual_demand' + ) + + # Σ(inflows) + virtual_supply = Σ(outflows) + virtual_demand + eq_bus_balance.lhs += self.virtual_supply - self.virtual_demand @@ - self._model.effects.add_share_to_effects( - name=self.label_of_element, - expressions={PENALTY_EFFECT_LABEL: self.virtual_supply * imbalance_penalty}, - target='temporal', - ) - self._model.effects.add_share_to_effects( - name=self.label_of_element, - expressions={PENALTY_EFFECT_LABEL: self.virtual_demand * imbalance_penalty}, - target='temporal', - ) + total_imbalance = (self.virtual_supply + self.virtual_demand) * imbalance_penalty + self._model.effects.add_share_to_effects( + name=self.label_of_element, + expressions={PENALTY_EFFECT_LABEL: total_imbalance}, + target='temporal', + )This preserves behavior while making the intent more obvious.
Also applies to: 895-903
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)docs/user-guide/mathematical-notation/effects-penalty-objective.md(1 hunks)docs/user-guide/mathematical-notation/elements/Bus.md(2 hunks)examples/02_Complex/complex_example.py(2 hunks)examples/03_Optimization_modes/example_optimization_modes.py(2 hunks)flixopt/elements.py(8 hunks)flixopt/flow_system.py(1 hunks)flixopt/optimization.py(1 hunks)tests/test_bus.py(4 hunks)tests/test_functional.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/test_functional.py (1)
flixopt/elements.py (1)
Bus(183-297)
flixopt/optimization.py (1)
flixopt/elements.py (1)
allows_imbalance(292-293)
examples/03_Optimization_modes/example_optimization_modes.py (1)
flixopt/elements.py (1)
Bus(183-297)
examples/02_Complex/complex_example.py (1)
flixopt/elements.py (1)
Bus(183-297)
tests/test_bus.py (1)
flixopt/elements.py (1)
Bus(183-297)
⏰ 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 (10)
CHANGELOG.md (1)
124-135: LGTM! Comprehensive changelog documentation.The changelog clearly documents:
- The breaking change in default value (None vs 1e5)
- All renamed attributes and properties
- The deprecation path for the old parameter name
This provides users with clear migration guidance.
flixopt/flow_system.py (1)
80-80: LGTM! Documentation example updated correctly.The example in the FlowSystem docstring correctly uses the new
imbalance_penalty_per_flow_hourparameter name.flixopt/optimization.py (1)
309-322: LGTM! Implementation correctly uses new API.The implementation correctly:
- References
virtual_supplyandvirtual_demandinstead of the oldexcess_input/excess_output- Uses
bus.allows_imbalanceproperty instead of the oldwith_excess- Applies appropriate numerical tolerance (1e-3) when checking for non-zero imbalances
The logic correctly filters to only report buses that both allow imbalances AND have actual non-zero imbalances in the solution.
docs/user-guide/mathematical-notation/effects-penalty-objective.md (1)
200-200: LGTM! Documentation correctly updated.The penalty usage description correctly references the new
imbalance_penalty_per_flow_hourparameter name.examples/03_Optimization_modes/example_optimization_modes.py (1)
44-44: LGTM! Example correctly updated to use new API.The example correctly:
- Renames the variable from
excess_penaltytoimbalance_penaltyfor clarity- Uses the new
imbalance_penalty_per_flow_hourparameter in all Bus constructors- Maintains the same behavior with explicit value of 1e5
Also applies to: 70-73
tests/test_functional.py (1)
69-70: LGTM! Test correctly updated to use new API.The test setup correctly uses the new
imbalance_penalty_per_flow_hour=Noneparameter, which appropriately tests the strict balance constraint behavior (no imbalances allowed).examples/02_Complex/complex_example.py (1)
16-16: LGTM! Example correctly updated to use new API.The example correctly:
- Renames the variable from
excess_penaltytoimbalance_penaltyfor improved clarity- Uses the new
imbalance_penalty_per_flow_hourparameter in all Bus constructors- Maintains consistent behavior with explicit value of 1e5
Also applies to: 37-39
tests/test_bus.py (1)
9-16: Constructor rename usage looks correctAll Bus instantiations now use
imbalance_penalty_per_flow_hourwithNoneand large positive values, matching the new API and doc semantics (hard vs soft balance). No issues here.Also applies to: 28-36, 82-90
flixopt/elements.py (2)
196-236: Bus docstring and examples are in sync with new imbalance behaviorThe updated docstring and examples for
imbalance_penalty_per_flow_hour, the virtual balance equation, and theNonevs> 0semantics match what BusModel actually implements withvirtual_supplyandvirtual_demand. This keeps the public API documentation aligned with the modeling logic.
906-918: Including virtual_supply/virtual_demand in results_structure is consistent with balance semanticsConditionally appending
virtual_supplytoinputsandvirtual_demandtooutputswhen those variables exist lines up with the balance equation and keeps results for strict buses unchanged. This should make downstream result consumers see virtual imbalance terms as just another input/output, which is intuitive.
…ee typos:
- "a imbalance_penalty_per_flow_hour" → "an imbalance_penalty_per_flow_hour"
- "usefull" → "useful"
- "ifeasiblity" → "infeasibility"
2. tests/test_bus.py - Updated comments to use the new imbalance terminology instead of the old "excess" terminology
3. flixopt/elements.py (BusModel) - Improved code clarity:
- Changed eq_bus_balance.lhs -= -self.virtual_supply + self.virtual_demand to the more readable eq_bus_balance.lhs += self.virtual_supply -
self.virtual_demand
- Added a comment explaining the equation: # Σ(inflows) + virtual_supply = Σ(outflows) + virtual_demand
- Combined the two separate add_share_to_effects calls into a single call with the combined expression (self.virtual_supply +
self.virtual_demand) * imbalance_penalty
All 12 bus tests pass with these changes.
* | File | Changes |
|-------------------------|----------------------------------------------------------------------------------|
| flixopt/elements.py | Renamed attributes excess_input → virtual_supply, excess_output → virtual_demand |
| flixopt/optimization.py | Updated attribute access and result keys |
| tests/test_bus.py | Updated variable name strings in assertions |
| docs/.../Bus.md | Updated description of φ symbols |
The variable names in the optimization model are now:
- {BusName}|virtual_supply (was excess_input)
- {BusName}|virtual_demand (was excess_output)
* Renamed excess_penalty_per_flow_hour → imbalance_penalty_per_flow_hour
* rename excess_penalty to imbalance_penalty
* Change default to None
* Added self._validate_kwargs(kwargs) to catch typos and unexpected arguments
* Renamed with_excess → allows_imbalance
* Fix docstring
* 1. docs/user-guide/mathematical-notation/elements/Bus.md - Fixed three typos:
- "a imbalance_penalty_per_flow_hour" → "an imbalance_penalty_per_flow_hour"
- "usefull" → "useful"
- "ifeasiblity" → "infeasibility"
2. tests/test_bus.py - Updated comments to use the new imbalance terminology instead of the old "excess" terminology
3. flixopt/elements.py (BusModel) - Improved code clarity:
- Changed eq_bus_balance.lhs -= -self.virtual_supply + self.virtual_demand to the more readable eq_bus_balance.lhs += self.virtual_supply -
self.virtual_demand
- Added a comment explaining the equation: # Σ(inflows) + virtual_supply = Σ(outflows) + virtual_demand
- Combined the two separate add_share_to_effects calls into a single call with the combined expression (self.virtual_supply +
self.virtual_demand) * imbalance_penalty
All 12 bus tests pass with these changes.
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Refactor
excess_penalty_per_flow_hourtoimbalance_penalty_per_flow_hourfor improved claritywith_excesstoallows_imbalanceDocumentation
✏️ Tip: You can customize this high-level summary in your review settings.