Conversation
|
@JonaGL
I would rather be able to define the costs as prices:
But this leads to quite imprecise and unexpected linearization. |
tests/test_linear_converter.py
Outdated
| model.constraints['Converter|Converter(input)|flow_rate|single_segment'], | ||
| sum([model.variables[f'Converter|Piece_{i}|inside_piece'] | ||
| for i in range(len(piecewise_model.pieces))]) <= 1 | ||
| for i in range(len(piecewise_model.pieces))]) == 1 |
There was a problem hiding this comment.
Here is the change in LienarConversion
FBumann
left a comment
FBumann
left a comment
There was a problem hiding this comment.
Document the behavior change in LinearConversion
|
Erstmal von mir ein großes Danke, dass Du das so schnell in Angriff genommen hast. Ich bin allerdings in fünf Minuten auf dem Weg nach Hause und dann direkt mit Familie auf dem Weg zum Flughafen. |
oh ja, stimmt. Schönen Urlaub. |
WalkthroughAdds a new PiecewiseEffectsPerFlowHour interface and model, re-exports it, integrates it into Flow (new optional parameter, transform_data propagation, and FlowModel modeling), adjusts piecewise zero-point constraint semantics, updates changelog and CI release-note flow, and adds tests and fixtures for piecewise per-flow-hour effects. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test / Caller
participant F as Flow
participant IF as PiecewiseEffectsPerFlowHour
participant FS as FlowSystem
participant M as FlowModel
participant PEM as PiecewiseEffectsPerFlowHourModel
participant SM as SystemModel (linopy)
T->>F: instantiate Flow(..., piecewise_effects_per_flow_hour=IF)
T->>F: F.transform_data(FS)
alt IF present
F->>IF: IF.transform_data(FS, name_prefix)
IF-->>FS: register piecewise_flow_rate & piecewise_shares
end
T->>M: build model for Flow
alt IF provided
M->>PEM: construct(piecewise_origin, piecewise_shares, zero_point)
PEM->>SM: add variables (inside_piece, lambdas, per-effect totals)
PEM->>SM: add constraints (lambda linkage, segment constraints, operation equations)
note right of PEM: Zero-point handling:\nNone/False → sum(inside_piece) == 1\nTrue or linopy.Var → sum(inside_piece) <= zero_point
end
T->>SM: solve
SM-->>T: solution (flow rates, per-hour effects, operation totals)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
# Conflicts: # tests/test_linear_converter.py
…-flows' into feature/300-piecewiseeffects-for-flows
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
flixopt/components.py (1)
27-140: Docstring content is solid; two small polish nits
- Consider removing the hardcoded version reference “clarified in v2.1.7” to avoid future drift, or replace with “clarified in Unreleased” until tagged.
- If your docs are rendered with Sphinx/numpydoc (as suggested elsewhere), prefer “code-block:: python” over triple backticks to ensure proper formatting in HTML/PDF builds.
Apply this diff to the version reference:
- This behavior was clarified in v2.1.7 to prevent optimization edge cases. + This behavior was clarified in the Unreleased changes to prevent optimization edge cases.CHANGELOG.md (1)
14-14: Minor wording tweak for Added entry (optional)Add a hyphen for readability.
-- Added new Interface `PiecewiseEffectsPerFlowHour` to model non-linear relations between flow rates and effects. [[#310](https://github.com/flixOpt/flixopt/pull/310) by [@FBumann](https://github.com/FBumann)] +- Added a new interface `PiecewiseEffectsPerFlowHour` to model non-linear relations between flow rates and effects. [[#310](https://github.com/flixOpt/flixopt/pull/310) by [@FBumann](https://github.com/FBumann)]tests/conftest.py (1)
287-299: Fixture addition is coherent; add a brief comment for intent (optional)Values in
piecewise_sharesare absolute effect amounts per flow-hour, not unit prices. A one-line comment will prevent confusion for future readers.fx.Flow( 'Q_fu', bus='Gas', + # Effects per flow-hour are modeled as absolute amounts (not unit rates) via piecewise curves. piecewise_effects_per_flow_hour=fx.PiecewiseEffectsPerFlowHour(flixopt/elements.py (2)
254-255: Transform propagation is correct; consider early validation (optional)You rely on
PiecewiseEffectsPerFlowHourModelto assert equal piece counts across origin/shares. An earlier, user-facing check here would yield clearer errors.if self.piecewise_effects_per_flow_hour is not None: - self.piecewise_effects_per_flow_hour.transform_data(flow_system, self.label_full) + # Optional early validation: ensure consistent number of pieces + pw = self.piecewise_effects_per_flow_hour + n = len(pw.piecewise_flow_rate) + for name, s in pw.piecewise_shares.items(): + if len(s) != n: + raise PlausibilityError( + f'{self.label_full}: piece count mismatch for "{name}": ' + f'{len(s)} != {n} (flow_rate).' + ) + pw.transform_data(flow_system, self.label_full)
406-417: Piecewise effects integration: good; avoid accidental double-counting (optional)If an effect appears in both
effects_per_flow_hourandpiecewise_effects_per_flow_hour, it will be summed. If that’s unintended, guard against duplicates or document it.if self.element.piecewise_effects_per_flow_hour is not None: + # Optional: prevent duplicate effect labels across linear and piecewise shares + dup = set(self.element.effects_per_flow_hour.keys()) & set( + self.element.piecewise_effects_per_flow_hour.piecewise_shares.keys() + ) + if dup: + raise PlausibilityError( + f'{self.label_full}: effect(s) defined twice (linear and piecewise): {sorted(dup)}' + ) self.piecewise_effects = self.add( PiecewiseEffectsPerFlowHourModel( model=self._model, label_of_element=self.label_of_element, piecewise_origin=(self.flow_rate.name, self.element.piecewise_effects_per_flow_hour.piecewise_flow_rate), piecewise_shares=self.element.piecewise_effects_per_flow_hour.piecewise_shares, zero_point=self.on_off.on if self.on_off is not None else False, ), ) self.piecewise_effects.do_modeling()Alternatively, add a short note in Flow’s docstring that linear and piecewise shares are additive by design.
flixopt/features.py (4)
844-856: Fix typos and clarify zero_point semantics in docstringMinor typos and phrasing could confuse readers.
- zero_point: A variable that can be used to define a zero point for the Piecewise relation. - If None or False, no zero point is defined. THis leads to 0 not being possible, - unless its its contained in a Piece. + zero_point: Controls whether the piecewise relation may take the value 0. + If a linopy.Variable is provided, enforce sum(inside_piece) <= zero_point (per time step if time series). + If True, create a binary zero_point and enforce sum(inside_piece) <= zero_point. + If None or False, enforce sum(inside_piece) == 1, i.e., 0 is not possible unless 0 lies within a Piece.
899-921: Zero-point constraint: compute once, avoid duplicate constraints per variableYou recompute sign/rhs and add the identical single-segment constraint once per variable. It’s redundant and increases constraint count. Compute sign/rhs once outside the loop and add the constraint once.
Note: tests currently expect per-variable single_segment constraints; adjust tests only if you choose to deduplicate.
- for var_name in self._piecewise_variables: - variable = self._model.variables[var_name] - ... - if isinstance(self._zero_point, linopy.Variable): - self.zero_point = self._zero_point - sign, rhs = '<=', self.zero_point - elif self._zero_point is True: - self.zero_point = self.add( - self._model.add_variables( - coords=self._model.coords, binary=True, name=f'{self.label_full}|zero_point' - ), - 'zero_point', - ) - sign, rhs = '<=', self.zero_point - else: - sign, rhs = '=', 1 - - self.add( - self._model.add_constraints( - sum([piece.inside_piece for piece in self.pieces]), - sign, - rhs, - name=f'{self.label_full}|{variable.name}|single_segment', - ), - f'{var_name}|single_segment', - ) + # Determine zero-point behavior once + if isinstance(self._zero_point, linopy.Variable): + self.zero_point = self._zero_point + sign, rhs = '<=', self.zero_point + elif self._zero_point is True: + self.zero_point = self.add( + self._model.add_variables( + coords=self._model.coords if self._as_time_series else None, + binary=True, + name=f'{self.label_full}|zero_point', + ), + 'zero_point', + ) + sign, rhs = '<=', self.zero_point + else: + sign, rhs = '=', 1 + + # Single-segment selection (once) + self.add( + self._model.add_constraints( + sum(piece.inside_piece for piece in self.pieces), + sign, + rhs, + name=f'{self.label_full}|single_segment', + ), + 'single_segment', + ) + + # Piecewise lambda relation per variable + for var_name in self._piecewise_variables: + variable = self._model.variables[var_name] + self.add( + self._model.add_constraints( + variable + == sum( + piece_model.lambda0 * piece_bounds.start + piece_model.lambda1 * piece_bounds.end + for piece_model, piece_bounds in zip(self.pieces, self._piecewise_variables[var_name], strict=False) + ), + name=f'{self.label_full}|{var_name}|lambda', + ), + f'{var_name}|lambda', + )Additionally, please verify dimensional compatibility when providing a scalar zero_point for as_time_series=True; broadcasting rules should be explicit in docs or enforced.
1053-1058: Minor formatting/readability of add() callParentheses make it a bit hard to parse; reorder arguments on one level.
- effect: self.add( - self._model.add_variables( - coords=None, - name=f'{self.label_full}|{effect}'), - f'{effect}' - ) + effect: self.add( + self._model.add_variables(coords=None, name=f'{self.label_full}|{effect}'), + f'{effect}', + )
1114-1121: Use consistent add() formatting and ensure coords correctnessFormatting nit as above; coords=self._model.coords is correct for per-timestep shares.
- effect: self.add( - self._model.add_variables( - coords=self._model.coords, - name=f'{self.label_full}|{effect}'), - f'{effect}' - ) + effect: self.add( + self._model.add_variables(coords=self._model.coords, name=f'{self.label_full}|{effect}'), + f'{effect}', + )flixopt/interface.py (3)
66-225: Great doc, but consider moving extensive examples to docs site and link from hereThe long narrative is helpful but inflates the module. Suggest extracting to user docs and keeping a concise summary here with a link.
259-351: Clarify wording: values are “per-hour effect amounts”, not “total amounts”To avoid confusion with unit prices and totals, suggest this phrasing.
- Values are `Piecewise` objects defining the absolute effect values (not rates/prices). + Values are `Piecewise` objects defining the per-hour effect amounts (not unit rates/prices). @@ - ⚠️ IMPORTANT: Values represent total effect amounts, not unit rates. - For a flow rate of X, the effect value is interpolated from the `Piecewise`. - This is NOT flow_rate × unit_price (which would be non-linear). + ⚠️ IMPORTANT: Values represent per-hour effect amounts determined by the flow rate + via linear interpolation within each Piece. This is NOT flow_rate × unit_price.Also add a one-liner noting that the model multiplies the per-hour amount by hours_per_step to obtain the per-timestep effect.
357-361: Tiny indent nit in transform_dataThe inner loop line is slightly misaligned.
- for name, piecewise in self.piecewise_shares.items(): - piecewise.transform_data(flow_system, f'{name_prefix}|PiecewiseEffectsPerFlowHour|{name}') + for name, piecewise in self.piecewise_shares.items(): + piecewise.transform_data(flow_system, f'{name_prefix}|PiecewiseEffectsPerFlowHour|{name}')tests/test_flow.py (1)
216-274: Optional shares scenario is well-designedForcing a piece off mid-horizon validates flexibility. Consider adding a companion test with zero_point=True (or passing an on-variable) to allow sum(inside_piece)=0 when off.
I can add a test ensuring sum(inside_piece) <= on(t) when zero_point is linked to an on-variable. Want me to draft it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)flixopt/__init__.py(1 hunks)flixopt/commons.py(2 hunks)flixopt/components.py(1 hunks)flixopt/elements.py(6 hunks)flixopt/features.py(6 hunks)flixopt/interface.py(2 hunks)tests/conftest.py(1 hunks)tests/test_flow.py(1 hunks)tests/test_linear_converter.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
flixopt/commons.py (1)
flixopt/interface.py (5)
OnOffParameters(429-523)Piece(22-36)Piecewise(40-61)PiecewiseEffects(239-255)PiecewiseEffectsPerFlowHour(258-360)
flixopt/__init__.py (1)
flixopt/interface.py (1)
PiecewiseEffectsPerFlowHour(258-360)
tests/test_flow.py (2)
tests/conftest.py (3)
basic_flow_system_linopy(448-464)create_linopy_model(436-439)assert_conequal(467-483)flixopt/interface.py (3)
PiecewiseEffectsPerFlowHour(258-360)Piecewise(40-61)Piece(22-36)
flixopt/interface.py (4)
flixopt/structure.py (3)
register_class_for_io(34-43)Interface(110-254)transform_data(115-117)flixopt/components.py (3)
transform_data(190-195)transform_data(292-310)transform_data(428-435)flixopt/elements.py (3)
transform_data(71-73)transform_data(119-122)transform_data(241-259)flixopt/flow_system.py (1)
transform_data(322-326)
tests/conftest.py (2)
flixopt/elements.py (1)
Flow(147-312)flixopt/interface.py (3)
PiecewiseEffectsPerFlowHour(258-360)Piecewise(40-61)Piece(22-36)
flixopt/elements.py (2)
flixopt/features.py (13)
PiecewiseEffectsPerFlowHourModel(1091-1150)on(750-751)do_modeling(45-74)do_modeling(246-290)do_modeling(384-444)do_modeling(487-567)do_modeling(668-726)do_modeling(794-830)do_modeling(866-921)do_modeling(957-990)do_modeling(1051-1088)do_modeling(1113-1150)do_modeling(1186-1193)flixopt/interface.py (9)
OnOffParameters(429-523)PiecewiseEffectsPerFlowHour(258-360)transform_data(34-36)transform_data(59-61)transform_data(233-235)transform_data(251-255)transform_data(357-360)transform_data(414-417)transform_data(474-492)
flixopt/features.py (3)
flixopt/interface.py (3)
Piece(22-36)Piecewise(40-61)items(230-231)flixopt/structure.py (8)
add(331-355)coords(102-103)label_full(278-279)label_full(385-391)Model(303-433)SystemModel(46-107)label(381-382)hours_per_step(94-95)flixopt/effects.py (2)
effects(304-305)add_share_to_effects(342-354)
⏰ 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.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (13)
flixopt/__init__.py (1)
21-22: Public export looks goodRe-exporting
PiecewiseEffectsPerFlowHourvia the package root is consistent with the rest of the API surface.tests/test_linear_converter.py (1)
372-378: Correct change: enforce exactly one active piece without OnOffSwitching from “<= 1” to “== 1” aligns the non-OnOff semantics with the zero-flow prohibition documented for piecewise conversion. Good.
flixopt/commons.py (1)
21-29: API export wiring is correctImport and all update for
PiecewiseEffectsPerFlowHourare consistent and non-breaking.flixopt/elements.py (4)
15-17: Imports updated appropriatelyBringing in
PiecewiseEffectsPerFlowHourModelandPiecewiseEffectsPerFlowHouris correct and scoped.
161-170: New Flow arg: backward-compatible and well-placedParameter is optional with sane default. Please ensure it’s included in any serialization/round-trip paths if Flow objects are saved/loaded elsewhere.
Would you like a quick scan script to confirm
piecewise_effects_per_flow_houris present in Flow IO (to_dict/from_dict/schema)?
184-185: Docstring aligns with semanticsThe brief arg doc matches the model behavior introduced in features. No action needed.
212-213: State assignment is fineStoring
piecewise_effects_per_flow_houron the Flow instance is straightforward.flixopt/features.py (4)
15-15: Public import looks goodAdding Piece and Piecewise here aligns features with interface usage. No issues.
1091-1108: New model: solid addition and consistent with namingConstructor and assertions align with PiecewiseEffectsModel and tests. Good.
1124-1131: Piecewise mapping: aligns origin and share piecesMerging origin and per-effect variables by name is correct. Ensure all Piecewise have equal lengths (already asserted upstream). LGTM.
1145-1151: Aggregation to operation effects is correctMultiplying per-flow-hour shares by hours_per_step and targeting 'operation' is the expected behavior. Tests cover this. LGTM.
tests/test_flow.py (2)
119-174: Model-build assertions cover variables/constraints wellGood coverage of lambda and single-segment constraints and effect aggregation to operation.
175-214: Solution test matches interpolation semanticsValidates that the per-hour share equals the interpolated value and operation totals scale by hours_per_step. Nice.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
flixopt/features.py (1)
844-857: Docstring: fix typos and tighten zero-point semantics.Minor typos and clarity.
- zero_point: A variable that can be used to define a zero point for the Piecewise relation. - If None or False, no zero point is defined. THis leads to 0 not being possible, - unless its its contained in a Piece. + zero_point: Controls whether 0 is admissible in addition to piece segments. + If None or False, no zero point is defined (0 is only possible if contained in a Piece). + If True or a binary linopy.Variable is provided, 0 becomes admissible.Please also document that this behavior change affects LinearConversion, as noted earlier.
🧹 Nitpick comments (2)
flixopt/elements.py (1)
184-185: Clarify docstring wording.Make axes explicit to avoid confusion with “flow hours”.
- piecewise_effects_per_flow_hour: piecewise relation between flow hours and effects + piecewise_effects_per_flow_hour: piecewise relation between flow rate (x-axis) and effects per flow hour (y-axis)flixopt/features.py (1)
1086-1143: Piecewise per-flow-hour model integration looks solid.
- Time-series shares with as_time_series=True.
- Effects added to operation with hours_per_step multiplier.
Two small suggestions:
- Consider a short_name in self.add(...) for easier introspection (e.g., 'segments').
- Add a quick test asserting 0 is feasible when OnOff.off=1 and infeasible when OnOff.on=1 with zero_point equality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
flixopt/elements.py(6 hunks)flixopt/features.py(5 hunks)flixopt/interface.py(2 hunks)tests/test_flow.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- flixopt/interface.py
- tests/test_flow.py
🧰 Additional context used
🧬 Code graph analysis (2)
flixopt/elements.py (1)
flixopt/interface.py (8)
PiecewiseEffectsPerFlowHour(259-361)transform_data(34-36)transform_data(59-61)transform_data(233-235)transform_data(251-255)transform_data(358-361)transform_data(415-418)transform_data(475-493)
flixopt/features.py (3)
flixopt/interface.py (3)
Piece(22-36)Piecewise(40-61)items(230-231)flixopt/structure.py (8)
add(331-355)coords(102-103)label_full(278-279)label_full(385-391)Model(303-433)SystemModel(46-107)label(381-382)hours_per_step(94-95)flixopt/effects.py (2)
effects(304-305)add_share_to_effects(342-354)
⏰ 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.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (7)
flixopt/elements.py (4)
15-16: New imports look correct.They align with the new model/type usage introduced below.
161-170: New Flow parameter added — confirm coexistence semantics.If both effects_per_flow_hour and piecewise_effects_per_flow_hour are set, they’ll both add operation shares. Verify that double-counting is intended.
211-213: LGTM: attribute wiring.Stores the provided piecewise configuration without side effects.
254-256: Good: data transformation is delegated.Ensures piece bounds become model-ready time series.
flixopt/features.py (3)
15-15: Import updates are correct.Needed by new piecewise classes.
915-921: OK — add_constraints(lhs, sign, rhs) usage is supported (no change required).linopy.Model.add_constraints(lhs, sign=None, rhs=None, name=...) is part of the public API and explicitly accepts passing lhs, sign and rhs as separate arguments; the docs and examples show both operator-style constraints and the three-argument form. (linopy.readthedocs.io)
887-890: No change needed — project requires Python ≥3.10 so zip(strict=...) is supported.pyproject.toml sets requires-python = ">=3.10"; using zip(..., strict=False) is valid (redundant but harmless). Occurrences: flixopt/features.py (the snippet) and flixopt/calculation.py:358.
Likely an incorrect or invalid review comment.
| if self.element.piecewise_effects_per_flow_hour is not None: | ||
| self.piecewise_effects = self.add( | ||
| PiecewiseEffectsPerFlowHourModel( | ||
| model=self._model, | ||
| label_of_element=self.label_of_element, | ||
| piecewise_origin=( | ||
| self.flow_rate.name, | ||
| self.element.piecewise_effects_per_flow_hour.piecewise_flow_rate, | ||
| ), | ||
| piecewise_shares=self.element.piecewise_effects_per_flow_hour.piecewise_shares, | ||
| zero_point=self.on_off.on if self.on_off is not None else False, | ||
| ), | ||
| ) | ||
| self.piecewise_effects.do_modeling() | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Zero-point handling risks infeasibility when On/Off is absent.
Passing False forces “exactly one segment active,” disallowing zero unless 0 is inside a piece. That can make otherwise valid flows (min=0) infeasible. Recommend allowing zero by default when no On/Off is present.
- zero_point=self.on_off.on if self.on_off is not None else False,
+ zero_point=self.on_off.on if self.on_off is not None else True,Would you prefer a Flow-level flag (e.g., piecewise_zero_allowed_when_off: bool = True) to make this explicit?
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "Listing flixopt module files (top-level):"
ls -la flixopt || true
echo
echo "Search for Piecewise* classes / identifiers..."
rg -n --hidden --no-ignore-vcs "PiecewiseEffectsPerFlowHourModel" -S || true
rg -n --hidden --no-ignore-vcs "PiecewiseEffects" -S || true
rg -n --hidden --no-ignore-vcs "piecewise_effects_per_flow_hour" -S || true
rg -n --hidden --no-ignore-vcs "piecewise" -S || true
echo
echo "Search for 'zero_point' usages:"
rg -n --hidden --no-ignore-vcs "zero_point" -S || true
echo
echo "Show flixopt/elements.py around the snippet (lines ~390-430):"
if [ -f flixopt/elements.py ]; then
nl -ba flixopt/elements.py | sed -n '390,430p' || true
else
echo "flixopt/elements.py not found"
fi
echo
echo "Search for classes with 'Piecewise' in their name (to find model implementation):"
rg -n --hidden --no-ignore-vcs "class .*Piecewise" -S || true
echo
echo "Search for constructor parameters that mention zero_point (def __init__ ... zero_point):"
rg -n --hidden --no-ignore-vcs "def __init__\([^\)]*zero_point" -S || true
echo
echo "Search for 'on_off' near piecewise-related code:"
rg -n --hidden --no-ignore-vcs "on_off" -S || trueLength of output: 53460
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== features.py 820-920 ==="
sed -n '820,920p' flixopt/features.py || true
echo
echo "=== features.py 1040-1140 ==="
sed -n '1040,1140p' flixopt/features.py || true
echo
echo "=== elements.py 392-422 ==="
sed -n '392,422p' flixopt/elements.py || true
echo
echo "=== components.py 528-540 ==="
sed -n '528,540p' flixopt/components.py || true
echo
echo "=== tests/test_linear_converter.py 420-440 ==="
sed -n '420,440p' tests/test_linear_converter.py || trueLength of output: 11083
Allow zero when On/Off is absent — fix in PiecewiseModel, not by passing True at call sites
Confirmed: call sites (e.g. flixopt/elements.py) pass False when no On/Off; PiecewiseModel.do_modeling treats non-True/non-variable as sum(insides) == 1, forcing a segment and making flows with min=0 infeasible. Switching call-sites to True would create an unconstrained binary zero_point (undesirable).
Recommended minimal fix: in flixopt/features.py -> PiecewiseModel.do_modeling change the else branch from
sign, rhs = '=', 1
to
sign, rhs = '<=', 1
so zero (no active segment) is allowed by default. Keep call-sites as zero_point=self.on_off.on if present. Review related call-sites: flixopt/elements.py, flixopt/components.py, flixopt/linear_converters.py.
Would you prefer an explicit Flow-level flag (e.g. piecewise_zero_allowed_when_off: bool) instead of changing the default?
| self.zero_point = self._zero_point | ||
| rhs = self.zero_point | ||
| sign, rhs = '<=', self.zero_point | ||
| elif self._zero_point is True: | ||
| self.zero_point = self.add( | ||
| self._model.add_variables( | ||
| coords=self._model.coords, binary=True, name=f'{self.label_full}|zero_point' | ||
| ), | ||
| 'zero_point', | ||
| ) | ||
| rhs = self.zero_point | ||
| sign, rhs = '<=', self.zero_point | ||
| else: | ||
| rhs = 1 | ||
| sign, rhs = '=', 1 | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Bug: single-segment constraint should be equality, not ≤.
With “<=”, on=1 still allows selecting no segment, decoupling the variable from the piecewise definition (variable can collapse to 0). Use equality to enforce “exactly one segment when allowed.”
- self.zero_point = self._zero_point
- sign, rhs = '<=', self.zero_point
+ self.zero_point = self._zero_point
+ sign, rhs = '==', self.zero_point
@@
- sign, rhs = '<=', self.zero_point
+ sign, rhs = '==', self.zero_point
@@
- sign, rhs = '=', 1
+ sign, rhs = '=', 1If you intended “≤” to allow both 0 and 1 segments when on=1, please confirm—this weakens the piecewise linkage.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching repo for 'zero_point' occurrences ==="
rg -n --hidden -S 'zero_point' || true
if [ -f flixopt/features.py ]; then
echo "=== Showing flixopt/features.py (lines 820-940) ==="
nl -ba flixopt/features.py | sed -n '820,940p'
else
echo "=== flixopt/features.py not found; listing files with 'features.py' in name ==="
rg -n --hidden --hidden 'features.py' || true
fiLength of output: 2206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Showing flixopt/features.py (lines 760-1140) ==="
sed -n '760,1140p' flixopt/features.py || true
echo
echo "=== Occurrences of sign/rhs/zero_point/on_off in flixopt/features.py (with context) ==="
rg -n -C3 -e 'sign, rhs' -e 'zero_point' -e 'on_off' flixopt/features.py || true
echo
echo "=== All occurrences of the token 'sign' with context ==="
rg -n -C5 '\bsign\b' flixopt/features.py || trueLength of output: 21979
Enforce equality for single-segment when zero_point exists.
Change the constraint sign from '<=' to '=' when self._zero_point is a Variable or True so the constraint is sum(pieces.inside_piece) = zero_point (otherwise on/zero_point = 1 can still permit zero segments). File: flixopt/features.py (around lines 899–912) — update both branches that set sign = '<=' to '='; keep the else branch as '=' 1.
🤖 Prompt for AI Agents
In flixopt/features.py around lines 900 to 912, the constraint sign is set to
'<=' when self._zero_point is a Variable or True, which allows
sum(pieces.inside_piece) to be less than zero_point and can permit zero
segments; change both branches that currently set sign = '<=' to set sign = '='
so the constraint becomes sum(pieces.inside_piece) = zero_point (leave the
existing else branch that sets sign, rhs = '=', 1 unchanged). Ensure both
branches assign sign = '=' and keep rhs as the corresponding zero_point variable
or expression.
# Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/python-app.yaml (2)
142-152: Tighten prerelease detection; prevent false positives and DRY with downstream usageThe regex will also match refs containing "rc" as a substring (e.g., "...-marcus"). Tighten the boundary and keep this as the single source of truth for the later release step.
Apply:
- if [[ "${{ github.ref }}" =~ (alpha|beta|rc) ]]; then + if [[ "${{ github.ref }}" =~ (^|[-.])(alpha|beta|rc)([0-9.]*$) ]]; then echo "is_prerelease=true" >> $GITHUB_OUTPUT echo "✅ Detected pre-release" else echo "is_prerelease=false" >> $GITHUB_OUTPUT echo "✅ Detected stable release" fiOptionally, reference this output in the release step:
- prerelease: ${{ contains(github.ref, 'alpha') || contains(github.ref, 'beta') || contains(github.ref, 'rc') }} + prerelease: ${{ steps.prerelease.outputs.is_prerelease == 'true' }}
195-202: Avoid mixing body_path with auto-generated notesWhen body_path is provided, enabling generate_release_notes can duplicate or reorder content. Prefer a single source to keep notes predictable; also reuse the prerelease output to avoid logic drift.
- body_path: current_release_notes.md - draft: false - prerelease: ${{ contains(github.ref, 'alpha') || contains(github.ref, 'beta') || contains(github.ref, 'rc') }} - generate_release_notes: true + body_path: current_release_notes.md + draft: false + prerelease: ${{ steps.prerelease.outputs.is_prerelease == 'true' }} + generate_release_notes: false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/python-app.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
.github/workflows/python-app.yaml (1)
scripts/extract_release_notes.py (1)
extract_release_notes(12-30)
⏰ 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). (8)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
| if [[ "${{ steps.prerelease.outputs.is_prerelease }}" == "true" ]]; then | ||
| echo "📝 Generating pre-release notes" | ||
| cat > current_release_notes.md << EOF | ||
| # Pre-release $VERSION | ||
|
|
||
| This is a pre-release version for testing and feedback. | ||
|
|
||
| ## Installation | ||
| \`\`\`bash | ||
| pip install flixopt==$VERSION --pre | ||
| \`\`\` | ||
|
|
||
| ## What's Changed | ||
| See the [unreleased section](https://github.com/flixOpt/flixopt/blob/main/CHANGELOG.md#unreleased) in the changelog for upcoming features and changes. | ||
|
|
||
| ## Feedback | ||
| Please report any issues or feedback on [GitHub Issues](https://github.com/flixOpt/flixopt/issues). | ||
| EOF | ||
| else |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Heredoc indentation makes headings render as code blocks in release notes
YAML indentation is persisted inside the heredoc, so “# Pre-release …” is emitted with 10 leading spaces and shows up as a code block on GitHub. Strip the indent or use a tab-stripping heredoc.
Minimal fix (strip the 10-space indent after write):
if [[ "${{ steps.prerelease.outputs.is_prerelease }}" == "true" ]]; then
echo "📝 Generating pre-release notes"
- cat > current_release_notes.md << EOF
+ cat > current_release_notes.md << EOF
# Pre-release $VERSION
This is a pre-release version for testing and feedback.
## Installation
\`\`\`bash
pip install flixopt==$VERSION --pre
\`\`\`
## What's Changed
See the [unreleased section](https://github.com/flixOpt/flixopt/blob/main/CHANGELOG.md#unreleased) in the changelog for upcoming features and changes.
## Feedback
Please report any issues or feedback on [GitHub Issues](https://github.com/flixOpt/flixopt/issues).
EOF
+ # Remove YAML indentation (10 spaces) to render Markdown headings correctly
+ sed -i 's/^ //' current_release_notes.mdAlternative (no sed): use a tab-indented heredoc with stripping:
- cat > current_release_notes.md << EOF
+ cat > current_release_notes.md <<- EOF…and prefix all heredoc content lines with a single tab instead of spaces.
📝 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.
| if [[ "${{ steps.prerelease.outputs.is_prerelease }}" == "true" ]]; then | |
| echo "📝 Generating pre-release notes" | |
| cat > current_release_notes.md << EOF | |
| # Pre-release $VERSION | |
| This is a pre-release version for testing and feedback. | |
| ## Installation | |
| \`\`\`bash | |
| pip install flixopt==$VERSION --pre | |
| \`\`\` | |
| ## What's Changed | |
| See the [unreleased section](https://github.com/flixOpt/flixopt/blob/main/CHANGELOG.md#unreleased) in the changelog for upcoming features and changes. | |
| ## Feedback | |
| Please report any issues or feedback on [GitHub Issues](https://github.com/flixOpt/flixopt/issues). | |
| EOF | |
| else | |
| if [[ "${{ steps.prerelease.outputs.is_prerelease }}" == "true" ]]; then | |
| echo "📝 Generating pre-release notes" | |
| cat > current_release_notes.md << EOF | |
| # Pre-release $VERSION | |
| This is a pre-release version for testing and feedback. | |
| ## Installation | |
| \`\`\`bash | |
| pip install flixopt==$VERSION --pre | |
| \`\`\` | |
| ## What's Changed | |
| See the [unreleased section](https://github.com/flixOpt/flixopt/blob/main/CHANGELOG.md#unreleased) in the changelog for upcoming features and changes. | |
| ## Feedback | |
| Please report any issues or feedback on [GitHub Issues](https://github.com/flixOpt/flixopt/issues). | |
| EOF | |
| # Remove YAML indentation (10 spaces) to render Markdown headings correctly | |
| sed -i 's/^ //' current_release_notes.md | |
| else |
🤖 Prompt for AI Agents
In .github/workflows/python-app.yaml around lines 158-176 the heredoc is
indented so the generated release notes include leading spaces (rendering
headings as code); fix by using a tab-stripping heredoc (change the opener to
<<-EOF and prefix each heredoc line with a single tab) so YAML indentation is
preserved but the written file has no leading spaces, or if you prefer a minimal
patch keep the current heredoc and run a strip step after writing (e.g., sed to
remove the 10-space prefix from current_release_notes.md) to remove the unwanted
indentation.
|
@JonaGL this is published as a pre release. Install it via pip install flixopt==2.2.0rc2 --preAlso See Release Notes |
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)
.github/workflows/python-app.yaml (1)
198-205: Use the step output for prerelease flag instead of re-parsing the refAvoids logic drift between detection and publication.
- name: Create GitHub Release uses: softprops/action-gh-release@v2 with: body_path: current_release_notes.md draft: false - prerelease: ${{ contains(github.ref, 'alpha') || contains(github.ref, 'beta') || contains(github.ref, 'rc') }} - generate_release_notes: true + prerelease: ${{ steps.prerelease.outputs.is_prerelease == 'true' }} + # Optional: only auto-generate when we didn't provide body_path + # generate_release_notes: false
♻️ Duplicate comments (2)
.github/workflows/python-app.yaml (2)
161-179: Heredoc indentation makes headings render as code blocks in release notesCurrent heredoc is indented by 10 spaces, so Markdown headings are rendered as code on GitHub. Strip indentation with a tab-stripping heredoc.
- cat > current_release_notes.md << EOF - # Pre-release $VERSION - - This is a pre-release version for testing and feedback. - - ## Installation - \`\`\`bash - pip install flixopt==$VERSION --pre - \`\`\` - - ## What's Changed - See the [unreleased section](https://github.com/flixOpt/flixopt/blob/main/CHANGELOG.md#unreleased) in the changelog for upcoming features and changes. - - ## Feedback - Please report any issues or feedback on [GitHub Issues](https://github.com/flixOpt/flixopt/issues). - EOF + # Use tab-stripping heredoc; prefix content lines with a single TAB + cat > current_release_notes.md <<- EOF + # Pre-release $VERSION + + This is a pre-release version for testing and feedback. + + ## Installation + ```bash + pip install flixopt==$VERSION --pre + ``` + + ## What's Changed + See the [unreleased section](https://github.com/flixOpt/flixopt/blob/main/CHANGELOG.md#unreleased) in the changelog for upcoming features and changes. + + ## Feedback + Please report any issues or feedback on [GitHub Issues](https://github.com/flixOpt/flixopt/issues). + EOFAlternative minimal patch after write (no tabs required):
cat > current_release_notes.md << EOF ... EOF + # Remove the 10-space YAML indent from each line + sed -i 's/^ //' current_release_notes.md
185-189: Apply the same indent fix to the stable-release fallback heredocThe fallback notes block has the same 10-space indent and will render as code. Use the tab-stripping heredoc (or post-strip with sed) here too.
- cat > current_release_notes.md << EOF - # Release $VERSION - - See the [full changelog](https://github.com/flixOpt/flixopt/blob/main/CHANGELOG.md) for details. - EOF + cat > current_release_notes.md <<- EOF + # Release $VERSION + + See the [full changelog](https://github.com/flixOpt/flixopt/blob/main/CHANGELOG.md) for details. + EOF
🧹 Nitpick comments (1)
.github/workflows/python-app.yaml (1)
158-160: Prefer GITHUB_REF_NAME for version extractionMore robust and readable; avoids relying on the full refs/tags prefix.
- VERSION=${GITHUB_REF#refs/tags/v} + VERSION=${GITHUB_REF_NAME#v}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/python-app.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
.github/workflows/python-app.yaml (1)
scripts/extract_release_notes.py (1)
extract_release_notes(12-30)
| - name: Check if pre-release | ||
| id: prerelease | ||
| run: | | ||
| if [[ "${{ github.ref }}" =~ (alpha|beta|rc) ]]; then | ||
| echo "is_prerelease=true" >> $GITHUB_OUTPUT | ||
| echo "✅ Detected pre-release" | ||
| else | ||
| echo "is_prerelease=false" >> $GITHUB_OUTPUT | ||
| echo "✅ Detected stable release" | ||
| fi | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify and harden pre-release detection; drive GH Release flag from this output
- Use GITHUB_REF_NAME (already provided by Actions) instead of the full ref.
- Tighten the match to cover both PEP 440 (e.g., 2.2.0rc1) and hyphenated semver (e.g., 2.2.0-rc.1).
- Make this the single source of truth and consume it in the GH release step to avoid drift.
- - name: Check if pre-release
+ - name: Check if pre-release
id: prerelease
run: |
- if [[ "${{ github.ref }}" =~ (alpha|beta|rc) ]]; then
+ # Use ref name (e.g., v2.2.0, v2.2.0rc1, v2.2.0-rc.1)
+ REF="${GITHUB_REF_NAME}"
+ if [[ "$REF" =~ (alpha|beta|rc) ]] || [[ "$REF" =~ -?(a|b|rc)[0-9]+ ]]; then
echo "is_prerelease=true" >> $GITHUB_OUTPUT
echo "✅ Detected pre-release"
else
echo "is_prerelease=false" >> $GITHUB_OUTPUT
echo "✅ Detected stable release"
fiFollow-up (in “Create GitHub Release” step): set prerelease: ${{ steps.prerelease.outputs.is_prerelease == 'true' }} so both places use the same signal.
📝 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.
| - name: Check if pre-release | |
| id: prerelease | |
| run: | | |
| if [[ "${{ github.ref }}" =~ (alpha|beta|rc) ]]; then | |
| echo "is_prerelease=true" >> $GITHUB_OUTPUT | |
| echo "✅ Detected pre-release" | |
| else | |
| echo "is_prerelease=false" >> $GITHUB_OUTPUT | |
| echo "✅ Detected stable release" | |
| fi | |
| - name: Check if pre-release | |
| id: prerelease | |
| run: | | |
| # Use ref name (e.g., v2.2.0, v2.2.0rc1, v2.2.0-rc.1) | |
| REF="${GITHUB_REF_NAME}" | |
| if [[ "$REF" =~ (alpha|beta|rc) ]] || [[ "$REF" =~ -?(a|b|rc)[0-9]+ ]]; then | |
| echo "is_prerelease=true" >> $GITHUB_OUTPUT | |
| echo "✅ Detected pre-release" | |
| else | |
| echo "is_prerelease=false" >> $GITHUB_OUTPUT | |
| echo "✅ Detected stable release" | |
| fi |
🤖 Prompt for AI Agents
.github/workflows/python-app.yaml around lines 145-155: the prerelease detection
currently inspects the full ref and uses a loose match; replace it to read
GITHUB_REF_NAME (already provided by Actions), test against a tightened regex
that matches both PEP 440 (e.g. 2.2.0rc1) and hyphenated semver pre-release
labels (e.g. 2.2.0-rc.1), and emit a single boolean output is_prerelease
(true/false) to GITHUB_OUTPUT as the canonical signal; then in the "Create
GitHub Release" step consume that output with prerelease: ${{
steps.prerelease.outputs.is_prerelease == 'true' }} so the GH release flag is
driven by this single source of truth.
@FBumann Many thanks for the implementation of this new feature! I checked it out and until now it worked very well. I implemented a pressure loss scenario and proved it against a simulation. I've used 50 I've experienced a small error. But I guess it's not connected to this feature. Still I want to write it down. Maybe it's good to have it on the radar:
|
Glad to hear this. About the Error: Thanks a lot |
3 participants
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores