Move validation to Submodel classes #473
Conversation
|
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 WalkthroughRefactors validation by replacing scattered Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FullModel as FullCalculation
participant ElementModel
participant _Validate as _validate()
participant Submodels
User->>FullModel: do_modeling()
activate FullModel
FullModel->>ElementModel: _do_modeling()
activate ElementModel
rect rgb(220,235,250)
Note over ElementModel,_Validate: Validation phase (new)
ElementModel->>_Validate: _validate()
_Validate-->>ElementModel: ok / raise Error
end
alt Validation OK
ElementModel->>Submodels: create submodels & modeling
Submodels-->>FullModel: modeling data
else Validation Error
_Validate-->>FullModel: raise ValueError with context
end
deactivate ElementModel
FullModel-->>User: model ready or error
deactivate FullModel
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_component.py (1)
20-41: Error message asserted intest_flow_label_checkdoesn’t match current validationWith the new
ComponentModel._validateimplementation (seeflixopt/elements.py), duplicate flow labels are rejected with aValueErrorwhose message starts withFlow names must be unique!. This test, however, assertsmatch='already exists in flows', which is unlikely to match the actual exception and will cause the test to fail once modeling hits that validation.Consider aligning the test with the new behavior, e.g. by:
- Relaxing to
pytest.raises(ValueError)(nomatch), or- Updating
matchto a substring of the new message (e.g.'Flow names must be unique'), depending on how stable you want the message contract to be.
🧹 Nitpick comments (3)
flixopt/effects.py (1)
648-676: EffectCollectionModel._validate correctly guards effect share configurationThe new
_validate()implementation sensibly:
- Reuses
calculate_effect_share_factors()to build the temporal/periodic share graphs,- Rejects share mappings that reference unknown effect labels, and
- Detects and reports circular temporal/periodic share dependencies before any modeling work starts.
The call from
_do_modeling()ensures these checks run up front, which is a good place for them. CI is currently failing on Ruff formatting for this file, so don’t forget to runruff format flixopt/effects.py(or the project-wide formatter) before merging.flixopt/elements.py (1)
864-873: ComponentModel duplicate-flow-label validation is appropriateThe new
_validatemethod inComponentModelcorrectly enforces uniqueness of flow labels within a component and reports any duplicates with a clear message listing the repeated labels. That’s a good match for catching configuration errors at modeling time.Note that tests asserting on the exception message should be aligned with this new wording (see
test_flow_label_checkintests/test_component.py).flixopt/components.py (1)
710-751: LinearConverterModel._validate is robust and well-scopedThe
_validateimplementation here cleanly enforces:
- Exactly one of
conversion_factorsorpiecewise_conversionmust be specified,- Degrees of freedom > 0 when using
conversion_factors, and- All flow labels used in
conversion_factorsactually belong to the component.The warning for variable
InvestParameterssizes withpiecewise_conversionis also a helpful guardrail. No functional issues spotted; just remember to runruff format flixopt/components.pyfor the CI formatting error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
flixopt/components.py(3 hunks)flixopt/effects.py(1 hunks)flixopt/elements.py(3 hunks)flixopt/structure.py(1 hunks)tests/test_component.py(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Python Package CI/CD
flixopt/effects.py
[error] 1-1: Ruff formatting failed. Run 'ruff format' to fix code style issues in this file.
flixopt/elements.py
[error] 1-1: Ruff formatting failed. Run 'ruff format' to fix code style issues in this file.
flixopt/components.py
[error] 1-1: Ruff formatting failed. Run 'ruff format' to fix code style issues in this file.
🔇 Additional comments (2)
flixopt/structure.py (1)
1578-1597: ElementModel validation hook and call order look correctCentralizing element-level validation in
ElementModel._validate()and invoking it from_do_modeling()before delegating toSubmodel._do_modeling()gives a clean, consistent entry point for all element submodels. The current override pattern in subclasses (callingsuper()._do_modeling()first) preserves this ordering, so validation will always run before any modeling logic.flixopt/elements.py (1)
493-505: Flow helpers (label_full,size_is_fixed,_format_invest_params) look consistentThe new helpers for rendering
label_full, determining whether size is fixed, and formattingInvestParametersare straightforward and align with existing naming in tests (e.g.Component(flow)pattern). No functional issues here.
| def _validate(self): | ||
| """Validate Transmission configuration for modeling.""" | ||
| super()._validate() | ||
|
|
||
| # Check bus connections for bidirectional transmission | ||
| if self.element.in2 is not None: | ||
| assert self.element.in2.bus == self.element.out1.bus, ( | ||
| f'Output 1 and Input 2 do not start/end at the same Bus: ' | ||
| f'{self.element.out1.bus=}, {self.element.in2.bus=}' | ||
| ) | ||
| if self.element.out2 is not None: | ||
| assert self.element.out2.bus == self.element.in1.bus, ( | ||
| f'Input 1 and Output 2 do not start/end at the same Bus: ' | ||
| f'{self.element.in1.bus=}, {self.element.out2.bus=}' | ||
| ) | ||
|
|
||
| # Validate balanced transmission requirements | ||
| if self.element.balanced: | ||
| if self.element.in2 is None: | ||
| raise ValueError('Balanced Transmission needs InvestParameters in both in-Flows') | ||
| if not isinstance(self.element.in1.size, InvestParameters) or not isinstance( | ||
| self.element.in2.size, InvestParameters | ||
| ): | ||
| raise ValueError('Balanced Transmission needs InvestParameters in both in-Flows') | ||
| if (self.element.in1.size.minimum_or_fixed_size > self.element.in2.size.maximum_or_fixed_size).any() or ( | ||
| self.element.in1.size.maximum_or_fixed_size < self.element.in2.size.minimum_or_fixed_size | ||
| ).any(): | ||
| raise ValueError( | ||
| f'Balanced Transmission needs compatible minimum and maximum sizes. ' | ||
| f'Got: {self.element.in1.size.minimum_size=}, {self.element.in1.size.maximum_size=}, ' | ||
| f'{self.element.in1.size.fixed_size=} and ' | ||
| f'{self.element.in2.size.minimum_size=}, {self.element.in2.size.maximum_size=}, ' | ||
| f'{self.element.in2.size.fixed_size=}.' | ||
| ) | ||
|
|
There was a problem hiding this comment.
Fix .any() usage and consider replacing asserts in TransmissionModel._validate
The intent of _validate is good, but there are two issues:
- Ambiguous truth-value checks with
.any()
For balanced transmissions, you do:
if (self.element.in1.size.minimum_or_fixed_size > self.element.in2.size.maximum_or_fixed_size).any() or (
self.element.in1.size.maximum_or_fixed_size < self.element.in2.size.minimum_or_fixed_size
).any():
...When the *_or_fixed_size properties are xarray DataArrays (or numpy arrays), .any() returns an array object. Using that directly in if triggers the same “truth value of an array is ambiguous” error seen elsewhere.
Safer reduction via NumPy would look like:
- if (self.element.in1.size.minimum_or_fixed_size > self.element.in2.size.maximum_or_fixed_size).any() or (
- self.element.in1.size.maximum_or_fixed_size < self.element.in2.size.minimum_or_fixed_size
- ).any():
+ if np.any(
+ np.asarray(
+ self.element.in1.size.minimum_or_fixed_size > self.element.in2.size.maximum_or_fixed_size
+ )
+ ) or np.any(
+ np.asarray(
+ self.element.in1.size.maximum_or_fixed_size < self.element.in2.size.minimum_or_fixed_size
+ )
+ ):
raise ValueError(
f'Balanced Transmission needs compatible minimum and maximum sizes. '
f'Got: {self.element.in1.size.minimum_size=}, {self.element.in1.size.maximum_size=}, '
f'{self.element.in1.size.fixed_size=} and '
f'{self.element.in2.size.minimum_size=}, {self.element.in2.size.maximum_size=}, '
f'{self.element.in2.size.fixed_size=}.'
)- Asserts for user configuration errors
The bus-consistency checks:
if self.element.in2 is not None:
assert self.element.in2.bus == self.element.out1.bus, (...)
if self.element.out2 is not None:
assert self.element.out2.bus == self.element.in1.bus, (...)will be silently disabled when Python is run with optimizations (-O). Since these are configuration errors rather than internal invariants, it would be more robust to raise a PlausibilityError or ValueError explicitly instead of using assert.
Also, when in2 is None but balanced=True, the current error message (“needs InvestParameters in both in-Flows”) is misleading—the primary problem is that in2 is missing. Consider clarifying that message when you touch this code next.
🤖 Prompt for AI Agents
In flixopt/components.py around lines 634 to 668, change the truth-value checks
and the asserts: replace the array .any() usage with a proper boolean reduction
(e.g., use numpy.any(...) on the underlying arrays or call
.values.any()/.any().item() to produce a Python bool) so the if condition
evaluates to a real bool, and replace the assert-based bus-consistency checks
with explicit exceptions (raise PlausibilityError or ValueError) so
configuration errors are never skipped under -O; also make the balanced/in2
missing error message explicit (e.g., "Balanced Transmission requires a second
input flow (in2)"), and keep the existing detailed size error text when raising
errors for incompatible ranges.
| def _validate(self): | ||
| """Validate Storage configuration for modeling.""" | ||
| super()._validate() | ||
|
|
||
| # Validate string values and set flag | ||
| initial_is_last = False | ||
| if isinstance(self.element.initial_charge_state, str): | ||
| if self.element.initial_charge_state == 'lastValueOfSim': | ||
| initial_is_last = True | ||
| else: | ||
| raise PlausibilityError( | ||
| f'initial_charge_state has undefined value: {self.element.initial_charge_state}' | ||
| ) | ||
|
|
||
| # Use new InvestParameters methods to get capacity bounds | ||
| if isinstance(self.element.capacity_in_flow_hours, InvestParameters): | ||
| minimum_capacity = self.element.capacity_in_flow_hours.minimum_or_fixed_size | ||
| maximum_capacity = self.element.capacity_in_flow_hours.maximum_or_fixed_size | ||
| else: | ||
| maximum_capacity = self.element.capacity_in_flow_hours | ||
| minimum_capacity = self.element.capacity_in_flow_hours | ||
|
|
||
| # Initial capacity should not constraint investment decision | ||
| minimum_initial_capacity = maximum_capacity * self.element.relative_minimum_charge_state.isel(time=0) | ||
| maximum_initial_capacity = minimum_capacity * self.element.relative_maximum_charge_state.isel(time=0) | ||
|
|
||
| # Only perform numeric comparisons if not using 'lastValueOfSim' | ||
| if not initial_is_last: | ||
| if (self.element.initial_charge_state > maximum_initial_capacity).any(): | ||
| raise PlausibilityError( | ||
| f'{self.element.label_full}: {self.element.initial_charge_state=} ' | ||
| f'is constraining the investment decision. Chosse a value above {maximum_initial_capacity}' | ||
| ) | ||
| if (self.element.initial_charge_state < minimum_initial_capacity).any(): | ||
| raise PlausibilityError( | ||
| f'{self.element.label_full}: {self.element.initial_charge_state=} ' | ||
| f'is constraining the investment decision. Chosse a value below {minimum_initial_capacity}' | ||
| ) | ||
|
|
||
| # Validate balanced flow requirements | ||
| if self.element.balanced: | ||
| if not isinstance(self.element.charging.size, InvestParameters) or not isinstance( | ||
| self.element.discharging.size, InvestParameters | ||
| ): | ||
| raise PlausibilityError( | ||
| f'Balancing charging and discharging Flows in {self.element.label_full} is only possible with Investments.' | ||
| ) | ||
|
|
||
| if (self.element.charging.size.minimum_size > self.element.discharging.size.maximum_size).any() or ( | ||
| self.element.charging.size.maximum_size < self.element.discharging.size.minimum_size | ||
| ).any(): | ||
| raise PlausibilityError( | ||
| f'Balancing charging and discharging Flows in {self.element.label_full} need compatible minimum and maximum sizes. ' | ||
| f'Got: {self.element.charging.size.minimum_size=}, {self.element.charging.size.maximum_size=} and ' | ||
| f'{self.element.discharging.size.minimum_size=}, {self.element.discharging.size.maximum_size=}.' | ||
| ) | ||
|
|
There was a problem hiding this comment.
Fix initial-capacity bounds and .any() usage in StorageModel._validate
There are two intertwined issues in the storage validation:
- Swapped min/max initial capacity formulas
Currently you compute:
minimum_initial_capacity = maximum_capacity * self.element.relative_minimum_charge_state.isel(time=0)
maximum_initial_capacity = minimum_capacity * self.element.relative_maximum_charge_state.isel(time=0)If minimum_capacity < maximum_capacity, this can easily yield minimum_initial_capacity > maximum_initial_capacity, and your subsequent checks:
if (self.element.initial_charge_state > maximum_initial_capacity).any():
...
if (self.element.initial_charge_state < minimum_initial_capacity).any():
...will incorrectly flag valid initial_charge_state values as “constraining the investment decision”.
To represent the union of feasible initial charge states across the capacity range, these should be:
- minimum_initial_capacity = maximum_capacity * self.element.relative_minimum_charge_state.isel(time=0)
- maximum_initial_capacity = minimum_capacity * self.element.relative_maximum_charge_state.isel(time=0)
+ minimum_initial_capacity = minimum_capacity * self.element.relative_minimum_charge_state.isel(time=0)
+ maximum_initial_capacity = maximum_capacity * self.element.relative_maximum_charge_state.isel(time=0)- Ambiguous truth-value checks with
.any()
As in other models, you’re using .any() on array-like objects directly in if conditions:
if (self.element.initial_charge_state > maximum_initial_capacity).any():
...
if (self.element.initial_charge_state < minimum_initial_capacity).any():
...
...
if (self.element.charging.size.minimum_size > self.element.discharging.size.maximum_size).any() or (
self.element.charging.size.maximum_size < self.element.discharging.size.minimum_size
).any():
...With xarray DataArrays (or numpy arrays), these .any() calls yield arrays, and if <array> raises the usual ambiguous truth-value error.
A safe version would be:
- if not initial_is_last:
- if (self.element.initial_charge_state > maximum_initial_capacity).any():
+ if not initial_is_last:
+ if np.any(
+ np.asarray(self.element.initial_charge_state > maximum_initial_capacity)
+ ):
raise PlausibilityError(
f'{self.element.label_full}: {self.element.initial_charge_state=} '
- f'is constraining the investment decision. Chosse a value above {maximum_initial_capacity}'
+ f'is constraining the investment decision. Choose a value above {maximum_initial_capacity}'
)
- if (self.element.initial_charge_state < minimum_initial_capacity).any():
+ if np.any(
+ np.asarray(self.element.initial_charge_state < minimum_initial_capacity)
+ ):
raise PlausibilityError(
f'{self.element.label_full}: {self.element.initial_charge_state=} '
- f'is constraining the investment decision. Chosse a value below {minimum_initial_capacity}'
+ f'is constraining the investment decision. Choose a value below {minimum_initial_capacity}'
)
@@
- if (self.element.charging.size.minimum_size > self.element.discharging.size.maximum_size).any() or (
- self.element.charging.size.maximum_size < self.element.discharging.size.minimum_size
- ).any():
+ if np.any(
+ np.asarray(self.element.charging.size.minimum_size > self.element.discharging.size.maximum_size)
+ ) or np.any(
+ np.asarray(self.element.charging.size.maximum_size < self.element.discharging.size.minimum_size)
+ ):
raise PlausibilityError(
f'Balancing charging and discharging Flows in {self.element.label_full} need compatible minimum and maximum sizes. '
f'Got: {self.element.charging.size.minimum_size=}, {self.element.charging.size.maximum_size=} and '
f'{self.element.discharging.size.minimum_size=}, {self.element.discharging.size.maximum_size=}.'
)This fixes both the logical bounds and the xarray-related runtime errors, while also correcting the small “Chosse” typo in the messages.
🤖 Prompt for AI Agents
flixopt/components.py lines ~801-857: the initial-capacity bounds are swapped
and the boolean checks use array-valued .any() leading to incorrect logic and
ambiguous truth-value errors; swap the formulas so minimum_initial_capacity =
minimum_capacity * relative_minimum_charge_state.isel(time=0) and
maximum_initial_capacity = maximum_capacity *
relative_maximum_charge_state.isel(time=0), change all conditional checks that
call .any() to produce a Python bool (e.g. append .any().item() or wrap with
bool(...)) for the initial_charge_state comparisons and the balanced-flow size
comparisons, and fix the typo “Chosse” → “Choose” in both error messages.
flixopt/elements.py
Outdated
| class FlowModel(ElementModel): | ||
| element: Flow # Type hint | ||
|
|
||
| def __init__(self, model: FlowSystemModel, element: Flow): | ||
| super().__init__(model, element) | ||
|
|
||
| def _validate(self): | ||
| """Validate Flow configuration for modeling.""" | ||
| super()._validate() | ||
|
|
||
| # Check relative_minimum <= relative_maximum | ||
| if (self.element.relative_minimum > self.element.relative_maximum).any(): | ||
| raise PlausibilityError( | ||
| self.element.label_full + ': Take care, that relative_minimum <= relative_maximum!' | ||
| ) | ||
|
|
||
| if not isinstance(self.size, InvestParameters) and ( | ||
| np.any(self.size == CONFIG.Modeling.big) and self.fixed_relative_profile is not None | ||
| ): # Default Size --> Most likely by accident | ||
| # Warn about default size with fixed_relative_profile | ||
| if not isinstance(self.element.size, InvestParameters) and ( | ||
| np.any(self.element.size == CONFIG.Modeling.big) and self.element.fixed_relative_profile is not None | ||
| ): | ||
| logger.warning( | ||
| f'Flow "{self.label_full}" has no size assigned, but a "fixed_relative_profile". ' | ||
| f'Flow "{self.element.label_full}" has no size assigned, but a "fixed_relative_profile". ' | ||
| f'The default size is {CONFIG.Modeling.big}. As "flow_rate = size * fixed_relative_profile", ' | ||
| f'the resulting flow_rate will be very high. To fix this, assign a size to the Flow {self}.' | ||
| f'the resulting flow_rate will be very high. To fix this, assign a size to the Flow {self.element}.' | ||
| ) | ||
|
|
||
| if self.fixed_relative_profile is not None and self.on_off_parameters is not None: | ||
| # Warn about fixed_relative_profile with on_off_parameters | ||
| if self.element.fixed_relative_profile is not None and self.element.on_off_parameters is not None: | ||
| logger.warning( | ||
| f'Flow {self.label_full} has both a fixed_relative_profile and an on_off_parameters.' | ||
| f'Flow {self.element.label_full} has both a fixed_relative_profile and an on_off_parameters. ' | ||
| f'This will allow the flow to be switched on and off, effectively differing from the fixed_flow_rate.' | ||
| ) | ||
|
|
||
| if np.any(self.relative_minimum > 0) and self.on_off_parameters is None: | ||
| # Warn about relative_minimum > 0 without on_off_parameters | ||
| if np.any(self.element.relative_minimum > 0) and self.element.on_off_parameters is None: | ||
| logger.warning( | ||
| f'Flow {self.label_full} has a relative_minimum of {self.relative_minimum} and no on_off_parameters. ' | ||
| f'This prevents the flow_rate from switching off (flow_rate = 0). ' | ||
| f'Flow {self.element.label_full} has a relative_minimum of {self.element.relative_minimum} ' | ||
| f'and no on_off_parameters. This prevents the flow_rate from switching off (flow_rate = 0). ' | ||
| f'Consider using on_off_parameters to allow the flow to be switched on and off.' | ||
| ) | ||
|
|
||
| if self.previous_flow_rate is not None: | ||
| # Validate previous_flow_rate type | ||
| if self.element.previous_flow_rate is not None: | ||
| if not any( | ||
| [ | ||
| isinstance(self.previous_flow_rate, np.ndarray) and self.previous_flow_rate.ndim == 1, | ||
| isinstance(self.previous_flow_rate, (int, float, list)), | ||
| isinstance(self.element.previous_flow_rate, np.ndarray) | ||
| and self.element.previous_flow_rate.ndim == 1, | ||
| isinstance(self.element.previous_flow_rate, (int, float, list)), | ||
| ] | ||
| ): | ||
| raise TypeError( | ||
| f'previous_flow_rate must be None, a scalar, a list of scalars or a 1D-numpy-array. Got {type(self.previous_flow_rate)}. ' | ||
| f'previous_flow_rate must be None, a scalar, a list of scalars or a 1D-numpy-array. ' | ||
| f'Got {type(self.element.previous_flow_rate)}. ' | ||
| f'Different values in different periods or scenarios are not yet supported.' | ||
| ) | ||
|
|
There was a problem hiding this comment.
Fix xarray/NumPy boolean reduction in FlowModel._validate
The current validation uses array-style .any() / np.any() directly in if conditions:
(self.element.relative_minimum > self.element.relative_maximum).any()np.any(self.element.size == CONFIG.Modeling.big)np.any(self.element.relative_minimum > 0)
Once relative_* and size have been transformed to model coordinates (xarray DataArray), these expressions usually yield DataArray objects, and if <DataArray> raises ValueError: the truth value of a DataArray is ambiguous. For scalar inputs, calling .any() on a plain bool will also fail.
This will cause _validate to crash for otherwise valid configurations as soon as flows are modeled.
You can avoid this by reducing via NumPy after converting to a plain ndarray, e.g.:
- if (self.element.relative_minimum > self.element.relative_maximum).any():
+ if np.any(
+ np.asarray(self.element.relative_minimum > self.element.relative_maximum)
+ ):
raise PlausibilityError(
self.element.label_full + ': Take care, that relative_minimum <= relative_maximum!'
)
@@
- if not isinstance(self.element.size, InvestParameters) and (
- np.any(self.element.size == CONFIG.Modeling.big) and self.element.fixed_relative_profile is not None
- ):
+ if not isinstance(self.element.size, InvestParameters) and (
+ np.any(np.asarray(self.element.size == CONFIG.Modeling.big))
+ and self.element.fixed_relative_profile is not None
+ ):
@@
- if np.any(self.element.relative_minimum > 0) and self.element.on_off_parameters is None:
+ if np.any(np.asarray(self.element.relative_minimum > 0)) and self.element.on_off_parameters is None:This keeps the semantics but ensures the conditions evaluate to plain booleans for both scalars and array-like inputs.
🤖 Prompt for AI Agents
In flixopt/elements.py around lines 507 to 562, the boolean checks use
DataArray/.any() and np.any() directly which can raise "truth value of a
DataArray is ambiguous" or fail on plain bools; convert the expressions to plain
numpy scalars/arrays before reducing and coerce to bool. Replace checks like
(self.element.relative_minimum > self.element.relative_maximum).any(),
np.any(self.element.size == CONFIG.Modeling.big) and
np.any(self.element.relative_minimum > 0) with reductions that first obtain a
numpy array (e.g. np.asarray(...) or .values) and then call np.any(...) and wrap
the result with bool(...) (or use np.asarray(...).any()) so the if conditions
always receive a plain Python boolean. Ensure both xarray DataArray and scalar
inputs are handled the same way.
| def _validate(self): | ||
| """Validate Bus configuration for modeling.""" | ||
| super()._validate() | ||
|
|
||
| # Check excess penalty value | ||
| if self.element.excess_penalty_per_flow_hour is not None: | ||
| zero_penalty = np.all(np.equal(self.element.excess_penalty_per_flow_hour, 0)) | ||
| if zero_penalty: | ||
| logger.warning( | ||
| f'In Bus {self.element.label_full}, the excess_penalty_per_flow_hour is 0. ' | ||
| f'Use "None" or a value > 0.' | ||
| ) | ||
|
|
||
| # Check bus has connected flows | ||
| if len(self.element.inputs) == 0 and len(self.element.outputs) == 0: | ||
| raise ValueError( | ||
| f'Bus "{self.element.label_full}" has no Flows connected to it. Please remove it from the FlowSystem' | ||
| ) | ||
|
|
There was a problem hiding this comment.
Fix ambiguous truth-value check in BusModel._validate
excess_penalty_per_flow_hour can be time-dependent and is transformed to an xarray DataArray. With:
zero_penalty = np.all(np.equal(self.element.excess_penalty_per_flow_hour, 0))
if zero_penalty:
...zero_penalty will often be a DataArray, and if zero_penalty: will raise a "truth value of an array is ambiguous" error. The same issue occurs for numpy arrays.
You can safely reduce to a scalar boolean via NumPy:
- if self.element.excess_penalty_per_flow_hour is not None:
- zero_penalty = np.all(np.equal(self.element.excess_penalty_per_flow_hour, 0))
- if zero_penalty:
+ if self.element.excess_penalty_per_flow_hour is not None:
+ zero_penalty = np.all(
+ np.asarray(self.element.excess_penalty_per_flow_hour == 0)
+ )
+ if zero_penalty:
logger.warning(
f'In Bus {self.element.label_full}, the excess_penalty_per_flow_hour is 0. '
f'Use "None" or a value > 0.'
)This preserves behavior for scalars and arrays without triggering xarray’s ambiguous truth-value error.
🤖 Prompt for AI Agents
In flixopt/elements.py around lines 798 to 816, replace the ambiguous
truth-value check of excess_penalty_per_flow_hour with an explicit scalar
boolean: compute the element-wise comparison (np.equal(..., 0)), reduce to a
single boolean with np.all(...), and coerce to a Python bool (e.g.,
bool(np.all(...))) before using it in the if statement so DataArray or ndarray
results no longer raise an ambiguous truth-value error.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Description
Move validation away from Components to Submodels
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Refactor
Bug Fixes
New Features
Tests