Feature/prevent simultanious flow rates #431
Conversation
WalkthroughThis PR adds nested-group support for prevent_simultaneous_flows (list[list[str]]), normalizes inputs to label-based groups (deprecates Flow objects), forwards the option to LinearConverter and component subclasses, updates modeling to create one mutual-exclusion constraint per group, and adds unit and IO roundtrip tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Component
participant Normalizer as _normalize_simultaneous_flows
participant ModelBuilder as ComponentModel
participant LinearConv as LinearConverter
User->>Component: __init__(prevent_simultaneous_flows=...)
Component->>Normalizer: normalize input
Note right of Normalizer #DDEBF7: Accept None, single group, or multiple groups\nConvert Flow objects -> labels (emit DeprecationWarning)
Normalizer-->>Component: list[list[str]] | None
alt component is a converter
User->>LinearConv: create with prevent_simultaneous_flows
LinearConv->>Component: forward label-based groups
end
Component->>ModelBuilder: for each group, ensure OnOffParameters per flow
ModelBuilder->>ModelBuilder: create one mutual-exclusion constraint per group
Note right of ModelBuilder #F3E6FF: Constraint names include group index when multiple
ModelBuilder-->>User: model with group-aware constraints
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
🚧 Files skipped from review as they are similar to previous changes (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)
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: 4
🧹 Nitpick comments (2)
tests/test_io.py (1)
75-161: Good round‑trip coverage for single and multiple groupsConsider adding negative cases to lock in error handling:
- Unknown label in prevent_simultaneous_flows should raise PlausibilityError at modeling.
- Groups with <2 unique labels should serialize to None or be skipped with a warning.
Happy to provide test stubs if you want.
tests/test_component.py (1)
604-797: Solid behavioral testsNice checks for constraint names and sums. Recommend two quick adds:
- Duplicate labels within a group produce deduped set (or explicit error).
- Single‑element group is skipped (or raises), matching the normalization guard.
This will prevent regressions around over‑tightening and assertion errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(2 hunks)flixopt/components.py(7 hunks)flixopt/elements.py(4 hunks)tests/test_component.py(1 hunks)tests/test_io.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/test_component.py (2)
tests/conftest.py (5)
basic_flow_system_linopy_coords(667-682)coords_config(70-72)create_linopy_model(721-733)assert_conequal(736-752)gas(92-93)flixopt/components.py (1)
LinearConverter(30-242)
flixopt/elements.py (2)
flixopt/interface.py (1)
OnOffParameters(1066-1331)flixopt/modeling.py (2)
ModelingPrimitives(197-387)mutual_exclusivity_constraint(345-387)
tests/test_io.py (3)
flixopt/flow_system.py (3)
FlowSystem(46-980)to_dataset(243-255)from_dataset(258-313)flixopt/components.py (1)
LinearConverter(30-242)flixopt/structure.py (2)
to_dataset(640-663)from_dataset(684-717)
flixopt/components.py (1)
flixopt/elements.py (1)
Flow(311-571)
⏰ 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.10)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
🔇 Additional comments (9)
flixopt/elements.py (2)
115-118: Normalization assignment looks goodStoring a normalized nested list early avoids branching later and simplifies IO.
867-874: Ensuring on/off binaries for grouped flows is correctThis guarantees binaries exist for groups without forcing them on unrelated flows.
CHANGELOG.md (3)
56-58: Nice, clear end‑user framingExamples for multiple groups are precise and match the implementation.
64-64: Deprecation note is helpfulExplicit example lowers friction for users migrating from Flow objects.
78-80: Good to call out type‑hint polish in Dev sectionKeeps readers aware of subtle runtime None cases.
flixopt/components.py (4)
175-179: Constructor forwarding is correctPassing prevent_simultaneous_flows straight to Component centralizes behavior.
409-413: Storage: mapping to labels is correctSingle group for charge/discharge is appropriate here.
669-672: Transmission: group only when bidirectional enabledGood conditional; only creates XOR when both directions exist and flag is True.
1088-1091: Semantics of SourceAndSink may be stricter than docThis creates a single group across inputs+outputs, enforcing “at most one flow active across all,” not just “no input and output simultaneously.” Confirm intent; if the goal is “source OR sink,” the proper formulation needs a mode binary, not a single mutual‑exclusion across all flows. Alternatively, adjust docs to match current behavior.
| prevent_simultaneous_flows=[flow.label for flow in outputs] if prevent_simultaneous_flow_rates else None, | ||
| ) |
There was a problem hiding this comment.
Bug: iteration on None when outputs=None and flag=True
Guard None to avoid TypeError.
- prevent_simultaneous_flows=[flow.label for flow in outputs] if prevent_simultaneous_flow_rates else None,
+ prevent_simultaneous_flows=[flow.label for flow in (outputs or [])]
+ if prevent_simultaneous_flow_rates
+ else None,🤖 Prompt for AI Agents
In flixopt/components.py around lines 1218-1219, the list comprehension uses
"outputs" without guarding for None when prevent_simultaneous_flow_rates is
True, causing a TypeError if outputs is None; change the expression to only
build the list when both prevent_simultaneous_flow_rates is truthy and outputs
is not None (e.g., use a conditional like " [flow.label for flow in outputs] if
prevent_simultaneous_flow_rates and outputs else None") so the field becomes
None instead of iterating over None.
| prevent_simultaneous_flows=[flow.label for flow in inputs] if prevent_simultaneous_flow_rates else None, | ||
| ) |
There was a problem hiding this comment.
Bug: iteration on None when inputs=None and flag=True
Guard None to avoid TypeError.
- prevent_simultaneous_flows=[flow.label for flow in inputs] if prevent_simultaneous_flow_rates else None,
+ prevent_simultaneous_flows=[flow.label for flow in (inputs or [])]
+ if prevent_simultaneous_flow_rates
+ else None,📝 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.
| prevent_simultaneous_flows=[flow.label for flow in inputs] if prevent_simultaneous_flow_rates else None, | |
| ) | |
| prevent_simultaneous_flows=[flow.label for flow in (inputs or [])] | |
| if prevent_simultaneous_flow_rates | |
| else None, | |
| ) |
🤖 Prompt for AI Agents
In flixopt/components.py around lines 1343 to 1344, the list comprehension
prevent_simultaneous_flows=[flow.label for flow in inputs] runs when inputs may
be None, causing a TypeError; fix by guarding inputs before iterating (e.g., set
prevent_simultaneous_flows to [flow.label for flow in inputs] if inputs is not
None and prevent_simultaneous_flow_rates else None, or use inputs or [] when
building the list) so no iteration occurs on None.
| # Create mutual exclusivity constraint for each group | ||
| # Each group enforces "at most 1 flow active at a time" | ||
| # flow_labels_group is a list of flow label strings | ||
| for group_idx, flow_labels_group in enumerate(self.element.prevent_simultaneous_flows): | ||
| constraint_name = ( | ||
| 'prevent_simultaneous_use' | ||
| if len(self.element.prevent_simultaneous_flows) == 1 | ||
| else f'prevent_simultaneous_use|group{group_idx}' | ||
| ) | ||
| # Look up flows by their labels and get their binary variables | ||
| flows_in_group = [self.element.flows[label] for label in flow_labels_group] | ||
| ModelingPrimitives.mutual_exclusivity_constraint( | ||
| self, | ||
| binary_variables=[flow.submodel.on_off.on for flow in flows_in_group], | ||
| short_name=constraint_name, | ||
| ) |
There was a problem hiding this comment.
Validate labels and guard small groups before creating constraints
Avoid KeyError on unknown labels and assertion on groups with <2 variables; also dedup here as a final safety net.
- for group_idx, flow_labels_group in enumerate(self.element.prevent_simultaneous_flows):
+ for group_idx, flow_labels_group in enumerate(self.element.prevent_simultaneous_flows):
+ # Dedup and validate labels early
+ unique_labels = list(dict.fromkeys(flow_labels_group))
+ missing = [lbl for lbl in unique_labels if lbl not in self.element.flows]
+ if missing:
+ raise PlausibilityError(
+ f'{self.label_full}: prevent_simultaneous_flows references unknown flows: {missing}. '
+ f'Available: {list(self.element.flows)}'
+ )
@@
- flows_in_group = [self.element.flows[label] for label in flow_labels_group]
+ flows_in_group = [self.element.flows[label] for label in unique_labels]
+ if len(flows_in_group) < 2:
+ warnings.warn(
+ f'{self.label_full}: skipping prevent_simultaneous_flows group {group_idx} '
+ f'with fewer than 2 unique flows.',
+ UserWarning,
+ )
+ continue🤖 Prompt for AI Agents
In flixopt/elements.py around lines 903 to 918, the loop creating mutual
exclusivity constraints must validate flow labels and guard tiny groups: first,
for each flow_labels_group, filter and deduplicate labels while checking
membership in self.element.flows (raise a clear ValueError listing unknown
labels or optionally log and skip the group); then build flows_in_group from
only the recognized labels; if fewer than 2 unique variables remain, skip
creating the constraint (or log a debug message) to avoid assertions; finally
pass the list of binary variables as before. Ensure the constraint_name logic is
unchanged and that any mutation preserves order if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
flixopt/elements.py (1)
915-930: Guard constraints against <2 flows and validate labels here as well.Avoid assertion from ModelingPrimitives when a group has fewer than 2 unique flows; dedup as final safety net. Mirrors prior feedback.
- for group_idx, flow_labels_group in enumerate(self.element.prevent_simultaneous_flows): + for group_idx, flow_labels_group in enumerate(self.element.prevent_simultaneous_flows): + # Dedup and validate labels as a final safety net + unique_labels = list(dict.fromkeys(flow_labels_group)) + missing = [lbl for lbl in unique_labels if lbl not in self.element.flows] + if missing: + raise PlausibilityError( + f'{self.label_full}: prevent_simultaneous_flows references unknown flows: {missing}. ' + f'Available: {list(self.element.flows)}' + ) @@ - flows_in_group = [self.element.flows[label] for label in flow_labels_group] + flows_in_group = [self.element.flows[label] for label in unique_labels] + if len(flows_in_group) < 2: + warnings.warn( + f'{self.label_full}: skipping prevent_simultaneous_flows group {group_idx} ' + f'with fewer than 2 unique flows.', + UserWarning, + ) + continue
🧹 Nitpick comments (6)
tests/test_component.py (2)
705-731: Deprecation path verified; internal normalization asserted.Nice use of pytest.warns. Consider also a nested-groups Flow-object case for completeness.
822-843: Test couples to a typo in error text.The message uses “occure” (typo). Suggest correcting to “occur” in code and update this regex to match “occur”.
- with pytest.raises(ValueError, match='Flow names must not occure multiple times'): + with pytest.raises(ValueError, match=r'Flow names must not occur multiple times'):flixopt/elements.py (4)
53-77: Docs are clear; add tiny wording fixes.Consider “occur” (not “occure”), “available” (not “Availlable”), and “You can’t”. Keeps user‑facing messages polished.
104-118: Broaden accepted types in the public API.Support tuples at the top level for convenience and consistency with inner groups. No behavior change.
- prevent_simultaneous_flows: list[str | Flow] | list[list[str | Flow]] | None = None, + prevent_simultaneous_flows: ( + list[str | Flow] + | tuple[str | Flow, ...] + | list[list[str | Flow] | tuple[str | Flow, ...]] + ) | None = None,
120-171: Normalization: accept tuples variadically; drop inner import; guard tiny groups.Tighten type hints, remove local
import warnings, and safely skip groups with <2 labels to avoid downstream asserts. Keep duplicate/unknown checks in plausibility (tests rely on ValueError there).- def _normalize_simultaneous_flows( - prevent_simultaneous_flows: list[Flow | str] | list[list[Flow | str] | tuple[Flow | str]] | None, - ) -> list[list[str]] | None: + def _normalize_simultaneous_flows( + prevent_simultaneous_flows: ( + list[str | Flow] + | tuple[str | Flow, ...] + | list[list[str | Flow] | tuple[str | Flow, ...]] + | None + ), + ) -> list[list[str]] | None: @@ - import warnings - if prevent_simultaneous_flows is None: return None - elif not isinstance(prevent_simultaneous_flows, (list, tuple)): + elif not isinstance(prevent_simultaneous_flows, (list, tuple)): raise TypeError('prevent_simultaneous_flows must be a list or tuple') @@ - # Check if it's a list of lists/tuples (multiple groups) or a single list - if len(prevent_simultaneous_flows) > 0 and isinstance(prevent_simultaneous_flows[0], (list, tuple)): - # Multiple groups: [['flow1', 'flow2'], ['flow3', 'flow4']] - return [[extract_label(item) for item in group] for group in prevent_simultaneous_flows] - else: - # Single group: ['flow1', 'flow2', 'flow3'] - return [[extract_label(item) for item in prevent_simultaneous_flows]] + def _dedup_preserve_order(labels: list[str]) -> list[str]: + seen: set[str] = set() + out: list[str] = [] + for lbl in labels: + if lbl not in seen: + seen.add(lbl) + out.append(lbl) + return out + + # Multiple groups? + if len(prevent_simultaneous_flows) > 0 and isinstance(prevent_simultaneous_flows[0], (list, tuple)): + groups: list[list[str]] = [] + for group in prevent_simultaneous_flows: + labels = _dedup_preserve_order([extract_label(item) for item in group]) + if len(labels) >= 2: + groups.append(labels) + return groups or None + # Single group + labels = _dedup_preserve_order([extract_label(item) for item in prevent_simultaneous_flows]) + return [labels] if len(labels) >= 2 else NoneIf you prefer raising on <2 labels instead of skipping, I can adjust the diff and update/add tests accordingly.
195-206: Plausibility checks: fix typos in error messages.Improve polish without changing behavior; tests asserting on “occure” will need updating.
- raise ValueError( - f'Flow names must not occure multiple times in "prevent_simultaneous_flows"! Got {group}' - ) + raise ValueError( + f'Flow names must not occur multiple times in "prevent_simultaneous_flows"! Got {group}' + ) @@ - f'Flow name "{flow_name}" is not present in the component "{self.label_full}". You cant use it in "prevent_simultaneous_flows". Availlable flows: {list(self.flows)}' + f'Flow name "{flow_name}" is not present in the component "{self.label_full}". ' + f'You can’t use it in "prevent_simultaneous_flows". Available flows: {list(self.flows)}' )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
flixopt/elements.py(5 hunks)tests/test_component.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
flixopt/elements.py (4)
flixopt/flow_system.py (1)
flows(760-762)flixopt/structure.py (2)
label_full(878-879)label_full(1014-1015)flixopt/interface.py (1)
OnOffParameters(1066-1331)flixopt/modeling.py (2)
ModelingPrimitives(197-387)mutual_exclusivity_constraint(345-387)
tests/test_component.py (2)
tests/conftest.py (5)
basic_flow_system_linopy_coords(667-682)coords_config(70-72)create_linopy_model(721-733)assert_conequal(736-752)gas(92-93)flixopt/components.py (1)
LinearConverter(30-242)
⏰ 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.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
🔇 Additional comments (7)
tests/test_component.py (6)
604-645: Solid single-group coverage; constraint and binaries verified.Assertions match naming and semantics; no issues.
646-704: Good multi-group test; namespaced constraints per group.Covers independent groups and sums ≤ 1; looks correct.
731-749: Preferred string-label path exercised.LGTM as a no-warning baseline.
750-771: Multiple groups normalization check.LGTM.
772-798: None/empty case validated end-to-end.Constraint absence check is precise; good.
799-821: Error path for unknown labels is well‑scoped.Regex is stable. If we fix typos in messages later, this test should still pass.
Add one more case for a 1‑label group to define desired behavior (skip with warning vs. error). Want me to draft it?
flixopt/elements.py (1)
879-886: Ensuring on/off vars for referenced flows is correct.This guarantees binaries exist before building exclusivity constraints. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
flixopt/elements.py (2)
120-171: Harden normalization: type hints, dedup, and <2-flows guardThe normalization logic still needs the improvements suggested in the previous review:
- Accept tuples for single-group input in type hints
- Deduplicate labels within each group to avoid over-tightening constraints (e.g.,
['on_i', 'on_i']shouldn't create2*on_i ≤ 1)- Skip groups with fewer than 2 unique labels and warn, as they cannot create valid mutual exclusivity constraints
- The docstring example (line 139) claims deduplication happens, but the code doesn't implement it
Based on previous review feedback.
914-930: Validate labels and guard small groups before creating constraintsThe constraint creation logic still needs the safeguards from the previous review:
- Deduplicate
flow_labels_groupbefore lookup to avoid duplicate variables in constraint- Validate that all labels exist in
self.element.flowsbefore line 925 access (defensive check, though plausibility should catch this)- Guard against groups with < 2 flows before calling
mutual_exclusivity_constraint, which will assert on small groups (seemodeling.py~line 229)Based on previous review feedback.
🧹 Nitpick comments (1)
flixopt/elements.py (1)
153-159: Verify deprecation warning stacklevel points to user code.The
stacklevel=4may not correctly point to the user's call site. Typical call stack is:
- User code
Component.__init__(line 98)_normalize_simultaneous_flows(line 121)extract_label(line 148)With
stacklevel=4, the warning shows the location 4 frames up fromwarnings.warn, which might be inside_normalize_simultaneous_flowsorComponent.__init__rather than user code. Consider testing withstacklevel=5orstacklevel=6to ensure it points to the user's instantiation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
flixopt/elements.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/elements.py (4)
flixopt/flow_system.py (1)
flows(760-762)flixopt/structure.py (2)
label_full(878-879)label_full(1014-1015)flixopt/interface.py (1)
OnOffParameters(1066-1331)flixopt/modeling.py (2)
ModelingPrimitives(197-387)mutual_exclusivity_constraint(345-387)
⏰ 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.10)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (2)
flixopt/elements.py (2)
53-76: Excellent documentation of the new grouped constraint behavior.The examples clearly distinguish single vs. multiple constraint groups, and the use cases (boiler fuels, power plant fuel+cooling) make the feature intuitive.
879-885: Flow preparation logic looks correct.The iteration properly handles multiple groups and creates
OnOffParametersfor flows that need binary variables for mutual exclusivity. This relies on the plausibility checks (lines 201-205) having validated that all flow labels exist.
| if self.prevent_simultaneous_flows is not None: | ||
| for group in self.prevent_simultaneous_flows: | ||
| if len(set(group)) != len(group): | ||
| raise ValueError( | ||
| f'Flow names must not occure multiple times in "prevent_simultaneous_flows"! Got {group}' | ||
| ) | ||
| for flow_name in group: | ||
| if flow_name not in self.flows: | ||
| raise ValueError( | ||
| f'Flow name "{flow_name}" is not present in the component "{self.label_full}". You cant use it in "prevent_simultaneous_flows". Availlable flows: {list(self.flows)}' | ||
| ) | ||
|
|
There was a problem hiding this comment.
Add guard for groups with fewer than 2 flows in plausibility checks.
While the duplicate and existence checks are good, there's no validation that each group has at least 2 flows. The mutual_exclusivity_constraint (see modeling.py line ~229) asserts len(binary_variables) >= 2. A group with 0 or 1 flow will cause an assertion failure during modeling rather than a clear error message here.
Add after line 200:
raise ValueError(
f'Flow names must not occure multiple times in "prevent_simultaneous_flows"! Got {group}'
)
+ if len(set(group)) < 2:
+ raise ValueError(
+ f'Each prevent_simultaneous_flows group must contain at least 2 unique flows. '
+ f'Got {len(set(group))} in group: {group}'
+ )
for flow_name in group:🤖 Prompt for AI Agents
In flixopt/elements.py around lines 195 to 206, the plausibility checks for
prevent_simultaneous_flows lack a guard that each group contains at least two
flow names; add a check after the existing duplicate/existence checks (or before
modeling) that raises a ValueError if len(group) < 2 with a clear message naming
the component and the invalid group so users get an immediate, descriptive error
instead of an assertion failure later.
Description
Improve the flexibility of mutual exclusivity constraints in Components and add the option to the LinearConverter class
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
Deprecated
Bug Fixes / Validation
Tests
Documentation