Feature/excess rename v4 #499
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)
WalkthroughRenames Bus penalty API from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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)
🔇 Additional comments (5)
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)
docs/user-guide/mathematical-notation/elements/Bus.md (1)
13-24: Clean up LaTeX notation in balance and penalty equationsThere are a couple of small LaTeX issues here that are worth fixing for clarity:
- In the balance equation, remove the stray space in the subscript:
- \sum_{f_\text{in} \in \mathcal{F}\text{in}} p{f_ \text{in}}(\text{t}i) + \phi\text{in}(\text{t}_i) =
- \sum_{f_\text{in} \in \mathcal{F}\text{in}} p{f_\text{in}}(\text{t}i) + \phi\text{in}(\text{t}_i) =
- In the penalty term, wrap `\text` properly: ```diff - \text a_{b \rightarrow \Phi}(\text{t}_i) \cdot \Delta \text{t}_i + \text{a}_{b \rightarrow \Phi}(\text{t}_i) \cdot \Delta \text{t}_i
🧹 Nitpick comments (4)
CHANGELOG.md (1)
52-72: Changelog entries match implementation; consider adding removal version to deprecationThe new bullets for the Bus imbalance penalty default and the renames look consistent with the Bus implementation and behavior. As a minor consistency tweak with other sections, you could extend the deprecation bullet to mention the planned removal version (e.g. “…will be removed in v5.0.0”) if that is already decided.
flixopt/optimization.py (1)
329-342: Align “Buses with excess” reporting with virtual_supply/virtual_demand terminologyThe switch to
virtual_supply/virtual_demandinmain_resultsis consistent with the BusModel changes, and the filtering on non‑zero sums still works as before.For naming consistency with the new terminology, you might (optionally) rename the section key to something like
"Buses with imbalance"instead of"Buses with excess"in a future breaking release.flixopt/elements.py (2)
199-201: Docstring and examples match new parameter; consider terminology polishThe constructor docs and examples correctly describe
imbalance_penalty_per_flow_hourand itsNonedefault (hard balance). The note still uses “excess/deficit” language while the implementation/tests talk aboutvirtual_supply/virtual_demand; it’s still conceptually correct but you might want to align terminology here for clarity.Also applies to: 211-211, 220-220, 229-229, 233-236
952-953: Imbalance modeling and penalty shares are consistent; consider simplifying balance updateThe BusModel now:
- builds
imbalance_penalty = hours_per_step * imbalance_penalty_per_flow_hour,- adds non-negative
virtual_supplyandvirtual_demandvariables,- adjusts the balance so it effectively becomes
inputs - outputs + virtual_supply - virtual_demand == 0,- and adds temporal penalty shares for both virtual variables.
This matches the updated tests and yields the expected objective contributions. The only nit is the line:
eq_bus_balance.lhs -= -self.virtual_supply + self.virtual_demandwhich is algebraically correct but a bit hard to read. For clarity you might prefer:
eq_bus_balance.lhs += self.virtual_supply - self.virtual_demandleaving behavior unchanged.
Also applies to: 955-961, 963-963, 968-971, 973-976
📜 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(9 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 (4)
examples/02_Complex/complex_example.py (1)
flixopt/elements.py (1)
Bus(184-297)
examples/03_Optimization_modes/example_optimization_modes.py (1)
flixopt/elements.py (1)
Bus(184-297)
tests/test_functional.py (1)
flixopt/elements.py (1)
Bus(184-297)
tests/test_bus.py (1)
flixopt/elements.py (1)
Bus(184-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.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (14)
docs/user-guide/mathematical-notation/elements/Bus.md (1)
28-34: Virtual supply/demand terminology and coefficient name look consistentThe definitions of
$\phi_\text{in}$ /$\phi_\text{out}$ as virtual supply/demand and the mapping of$\text{a}_{b \rightarrow \Phi}$ toimbalance_penalty_per_flow_hourare clear and aligned with the Bus API.docs/user-guide/mathematical-notation/effects-penalty-objective.md (1)
193-207: Penalty usage note is accurate and well-scopedThe new “Primary usage” sentence correctly links penalties to
imbalance_penalty_per_flow_houron Buses and TSA flexibility; no changes needed.examples/02_Complex/complex_example.py (1)
13-40: Bus example now correctly usesimbalance_penalty_per_flow_hourThe local
imbalance_penaltyvariable and the three Bus initializations are aligned with the new parameter name and semantics; no further changes needed.examples/03_Optimization_modes/example_optimization_modes.py (1)
41-74: Imbalance penalty wiring in example is consistent with Bus APIThe renamed
imbalance_penaltyvariable and its use asimbalance_penalty_per_flow_hourfor all buses are consistent and clearly documented by the “or set to None if not needed” comment.flixopt/flow_system.py (1)
73-83: FlowSystem docstring Bus example updated correctlyThe example now uses
imbalance_penalty_per_flow_hour=1e4, which matches the new Bus constructor and docs.tests/test_functional.py (1)
64-80: Functional tests correctly updated to new Bus penalty keywordUsing
imbalance_penalty_per_flow_hour=Nonefor the two buses inflow_system_basematches the new Bus API and keeps the tests’ strict-balance assumption explicit.tests/test_bus.py (2)
12-12: Bus constructor updates align with new APIUsing
imbalance_penalty_per_flow_hourin all three test setups correctly tracks the renamed parameter, and explicitly passingNonewhere you want a hard balance is consistent with the new default. No issues from the test side here.Also applies to: 31-31, 85-85
40-41: Virtual supply/demand expectations and penalty expression look consistentThe updated expectations for:
- variable names (
TestBus|virtual_supply,TestBus|virtual_demand),- balance constraint (
Gastarif - Wärmelast + virtual_supply - virtual_demand == 0),- and penalty shares (
virtual_* * 1e5 * hours_per_step)all match the BusModel logic that augments the bus balance with
virtual_supply/virtual_demandand multiplies byhours_per_step * imbalance_penalty_per_flow_hour. This is a faithful adaptation of the previous excess-based tests.Also applies to: 48-48, 51-51, 58-59, 78-79
flixopt/elements.py (6)
250-251: Behavioral change: defaulting to a hard bus balance nowWith
imbalance_penalty_per_flow_hour: Numeric_TPS | None = Noneandwith_excessimplemented asself.imbalance_penalty_per_flow_hour is not None, a Bus without an explicit penalty now enforces a hard balance (no virtual variables, no slack). That’s stricter than a non-zero default penalty and may break existing models that relied on an implicit imbalance penalty.If this change is intentional, it’s fine as-is; otherwise, consider:
- restoring a positive default value, or
- clearly highlighting this as a breaking change in release notes / migration docs.
Also applies to: 293-293
275-277: Coordinate fitting for imbalance penalty looks consistentTransforming
imbalance_penalty_per_flow_hourvia_fit_coordsusing the bus prefix mirrors how other time/coords-dependent parameters are handled. As long as_fit_coordsalready acceptsNone, this is correct and consistent.
280-281: Zero-penalty plausibility warning is reasonableThe new check for
imbalance_penalty_per_flow_hourbeing identically zero and emitting a warning is a good guardrail against accidentally allowing free imbalances. The use ofnp.all(np.equal(..., 0))should work for both scalars and arrays.Also applies to: 284-285
291-293:with_excesssemantics align with imbalance penalty presenceBasing
with_excesspurely onimbalance_penalty_per_flow_hour is not Nonekeeps the modeling logic simple: virtual_supply/virtual_demand are only created when there is a (possibly zero) penalty parameter. This matches how BusModel gates imbalance handling and is internally consistent.
937-939: Initialization ofvirtual_supply/virtual_demandis cleanDeclaring
self.virtual_supplyandself.virtual_demandasNonebefore callingsuper().__init__makes their later optional use inresults_structurestraightforward and type-safe.
982-985: Including virtual supply/demand in results only when present makes senseAppending
virtual_supplytoinputsandvirtual_demandtooutputsconditionally on them being non-Noneis a nice way to keep the results structure minimal for hard-balance buses while still exposing imbalance variables when enabled.
| 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. | ||
|
|
There was a problem hiding this comment.
Fix grammar and spelling in imbalance penalty description
Consider tightening the wording and correcting typos, e.g.:
-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 penalizes imbalances from missing or excess flow-rates.
+This is useful as it handles potential infeasibility gently.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 penalizes imbalances from missing or excess flow-rates. | |
| This is useful as it handles potential infeasibility gently. | |
🧰 Tools
🪛 LanguageTool
[grammar] ~9-~9: Ensure spelling is correct
Context: ...r missing or excess flow-rates. This is usefull as it handles a possible ifeasiblity ge...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~9-~9: Ensure spelling is correct
Context: ...his is usefull as it handles a possible ifeasiblity gently. This changes the balance to $...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In docs/user-guide/mathematical-notation/elements/Bus.md around lines 8 to 10,
the description of the imbalance_penalty_per_flow_hour parameter contains typos
and unclear wording; rewrite the sentence to correct spelling (e.g., "penaltize"
→ "penalize", "usefull" → "useful", "ifeasiblity" → "infeasibility"), tighten
phrasing, and clarify the intent (for example: "Optionally, a Bus can have an
imbalance_penalty_per_flow_hour parameter, which penalizes deviations from
balance in flow rates and softly handles possible infeasibility."). Ensure
concise, grammatical English and keep technical meaning unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
CHANGELOG.md (1)
60-73: Changelog entry is accurate; consider clarifying deprecation/removal detailsThe bullets correctly describe the new default (
None→ strict balance) and the Bus/BusModel renames. Two minor polish points:
- In the Deprecated section, consider adding the planned removal version for
Bus.excess_penalty_per_flow_hour(consistent with other deprecations in this file).- For
Bus.with_excess→allows_imbalance, ifwith_excessis no longer available even as a deprecated alias, this is a breaking change and might be worth calling out explicitly under Breaking Changes rather than only under Changed.flixopt/elements.py (2)
193-237: Align Bus docstring balance equation with new virtual_supply/virtual_demand namingThe docstring still describes the balance as
Σ(inflows) = Σ(outflows) + excess - deficit, while the implementation and results now usevirtual_supplyandvirtual_demand. For clarity, consider updating the equation and surrounding text to something like:
Σ(inflows) + virtual_supply = Σ(outflows) + virtual_demandand consistently referring to “virtual supply/demand” instead of “excess/deficit” in this block.
280-295: Consider stricter plausibility checks on imbalance penaltiesRight now
_plausibility_checks()only warns when all entries ofimbalance_penalty_per_flow_hourare exactly zero, andallows_imbalancesimply checks foris not None.If negative penalties are never meaningful in this context, you may want to:
- Raise (or at least warn) when any value is
< 0, since that would make imbalances rewarding.- Optionally decide whether a mixed array (some zeros, some positive) should also trigger a warning, depending on whether “free” imbalances in some timesteps are intended.
The current behavior is safe but tightening these checks would catch obvious misconfigurations earlier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)flixopt/elements.py(8 hunks)flixopt/optimization.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- flixopt/optimization.py
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/elements.py (2)
flixopt/structure.py (5)
_handle_deprecated_kwarg(517-588)_validate_kwargs(590-618)_fit_coords(349-362)get_coords(227-256)get_coords(1445-1450)flixopt/flow_system.py (1)
coords(1120-1126)
🔇 Additional comments (2)
flixopt/elements.py (2)
247-278: Deprecation handling for Bus penalty keyword and data transformation look correctUsing
_handle_deprecated_kwargto mapexcess_penalty_per_flow_hour→imbalance_penalty_per_flow_hourand then calling_validate_kwargsensures:
- The old keyword keeps working with a deprecation path.
- Typos and unexpected kwargs still raise
TypeErrorinstead of being silently ignored.transform_data()correctly routesimbalance_penalty_per_flow_hourthrough_fit_coordsso it aligns with model coordinates.This mirrors the pattern used elsewhere (e.g.,
Flow.__init__) and avoids the earlier risk of swallowed kwargs. No changes needed from my side.
937-987: Virtual supply/demand modeling and penalty integration are consistent with the new APIThe BusModel changes look coherent:
- Initializing
virtual_supply/virtual_demandtoNonein__init__and only creating non‑negative variables whenelement.allows_imbalanceis true cleanly distinguishes hard vs. soft balance.- Updating the balance constraint via
eq_bus_balance.lhs -= -virtual_supply + virtual_demandyieldsΣ(inputs) + virtual_supply = Σ(outputs) + virtual_demand, which matches the intended semantics.- Multiplying by
hours_per_stepbefore applyingimbalance_penalty_per_flow_hourcorrectly converts the per‑flow‑hour penalty into a per‑timestep contribution.- Including the virtual variables in
results_structure()inputs/outputs only when they exist keeps result inspection aligned with the new terminology without affecting strictly balanced buses.I don’t see any issues here; this is a clean refactor.
Description
Rename bus variables and penalty parameter
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Breaking Changes
Deprecation
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.