Skip to content

Move validation to Submodel classes #473

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
FBumann wants to merge 4 commits into from
Closed

Move validation to Submodel classes #473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f8db355
Move validation to Submodel classes
FBumann Nov 17, 2025
f765e25
ruff format
FBumann Nov 17, 2025
ec0d7c6
Temp
FBumann Nov 17, 2025
10706ae
Move more validation to Model
FBumann Nov 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
249 changes: 134 additions & 115 deletions flixopt/components.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,6 @@ def __init__(
self.piecewise_conversion = piecewise_conversion

def create_model(self, model: FlowSystemModel) -> LinearConverterModel:
self._plausibility_checks()
self.submodel = LinearConverterModel(model, self)
return self.submodel

Expand All @@ -188,36 +187,6 @@ def _set_flow_system(self, flow_system) -> None:
if self.piecewise_conversion is not None:
self.piecewise_conversion._set_flow_system(flow_system)

def _plausibility_checks(self) -> None:
super()._plausibility_checks()
if not self.conversion_factors and not self.piecewise_conversion:
raise PlausibilityError('Either conversion_factors or piecewise_conversion must be defined!')
if self.conversion_factors and self.piecewise_conversion:
raise PlausibilityError('Only one of conversion_factors or piecewise_conversion can be defined, not both!')

if self.conversion_factors:
if self.degrees_of_freedom <= 0:
raise PlausibilityError(
f'Too Many conversion_factors_specified. Care that you use less conversion_factors '
f'then inputs + outputs!! With {len(self.inputs + self.outputs)} inputs and outputs, '
f'use not more than {len(self.inputs + self.outputs) - 1} conversion_factors!'
)

for conversion_factor in self.conversion_factors:
for flow in conversion_factor:
if flow not in self.flows:
raise PlausibilityError(
f'{self.label}: Flow {flow} in conversion_factors is not in inputs/outputs'
)
if self.piecewise_conversion:
for flow in self.flows.values():
if isinstance(flow.size, InvestParameters) and flow.size.fixed_size is None:
logger.warning(
f'Using a Flow with variable size (InvestParameters without fixed_size) '
f'and a piecewise_conversion in {self.label_full} is uncommon. Please verify intent '
f'({flow.label_full}).'
)

def transform_data(self, name_prefix: str = '') -> None:
prefix = '|'.join(filter(None, [name_prefix, self.label_full]))
super().transform_data(prefix)
Expand Down Expand Up @@ -436,7 +405,6 @@ def __init__(
self.balanced = balanced

def create_model(self, model: FlowSystemModel) -> StorageModel:
self._plausibility_checks()
self.submodel = StorageModel(model, self)
return self.submodel

Expand Down Expand Up @@ -485,62 +453,6 @@ def transform_data(self, name_prefix: str = '') -> None:
f'{prefix}|capacity_in_flow_hours', self.capacity_in_flow_hours, dims=['period', 'scenario']
)

def _plausibility_checks(self) -> None:
"""
Check for infeasible or uncommon combinations of parameters
"""
super()._plausibility_checks()

# Validate string values and set flag
initial_is_last = False
if isinstance(self.initial_charge_state, str):
if self.initial_charge_state == 'lastValueOfSim':
initial_is_last = True
else:
raise PlausibilityError(f'initial_charge_state has undefined value: {self.initial_charge_state}')

# Use new InvestParameters methods to get capacity bounds
if isinstance(self.capacity_in_flow_hours, InvestParameters):
minimum_capacity = self.capacity_in_flow_hours.minimum_or_fixed_size
maximum_capacity = self.capacity_in_flow_hours.maximum_or_fixed_size
else:
maximum_capacity = self.capacity_in_flow_hours
minimum_capacity = self.capacity_in_flow_hours

# Initial capacity should not constraint investment decision
minimum_initial_capacity = maximum_capacity * self.relative_minimum_charge_state.isel(time=0)
maximum_initial_capacity = minimum_capacity * self.relative_maximum_charge_state.isel(time=0)

# Only perform numeric comparisons if not using 'lastValueOfSim'
if not initial_is_last:
if (self.initial_charge_state > maximum_initial_capacity).any():
raise PlausibilityError(
f'{self.label_full}: {self.initial_charge_state=} '
f'is constraining the investment decision. Chosse a value above {maximum_initial_capacity}'
)
if (self.initial_charge_state < minimum_initial_capacity).any():
raise PlausibilityError(
f'{self.label_full}: {self.initial_charge_state=} '
f'is constraining the investment decision. Chosse a value below {minimum_initial_capacity}'
)

if self.balanced:
if not isinstance(self.charging.size, InvestParameters) or not isinstance(
self.discharging.size, InvestParameters
):
raise PlausibilityError(
f'Balancing charging and discharging Flows in {self.label_full} is only possible with Investments.'
)

if (self.charging.size.minimum_size > self.discharging.size.maximum_size).any() or (
self.charging.size.maximum_size < self.discharging.size.minimum_size
).any():
raise PlausibilityError(
f'Balancing charging and discharging Flows in {self.label_full} need compatible minimum and maximum sizes.'
f'Got: {self.charging.size.minimum_size=}, {self.charging.size.maximum_size=} and '
f'{self.discharging.size.minimum_size=}, {self.discharging.size.maximum_size=}.'
)

def __repr__(self) -> str:
"""Return string representation."""
# Use build_repr_from_init directly to exclude charging and discharging
Expand Down Expand Up @@ -697,34 +609,7 @@ def __init__(
self.absolute_losses = absolute_losses
self.balanced = balanced

def _plausibility_checks(self):
super()._plausibility_checks()
# check buses:
if self.in2 is not None:
assert self.in2.bus == self.out1.bus, (
f'Output 1 and Input 2 do not start/end at the same Bus: {self.out1.bus=}, {self.in2.bus=}'
)
if self.out2 is not None:
assert self.out2.bus == self.in1.bus, (
f'Input 1 and Output 2 do not start/end at the same Bus: {self.in1.bus=}, {self.out2.bus=}'
)

if self.balanced:
if self.in2 is None:
raise ValueError('Balanced Transmission needs InvestParameters in both in-Flows')
if not isinstance(self.in1.size, InvestParameters) or not isinstance(self.in2.size, InvestParameters):
raise ValueError('Balanced Transmission needs InvestParameters in both in-Flows')
if (self.in1.size.minimum_or_fixed_size > self.in2.size.maximum_or_fixed_size).any() or (
self.in1.size.maximum_or_fixed_size < self.in2.size.minimum_or_fixed_size
).any():
raise ValueError(
f'Balanced Transmission needs compatible minimum and maximum sizes.'
f'Got: {self.in1.size.minimum_size=}, {self.in1.size.maximum_size=}, {self.in1.size.fixed_size=} and '
f'{self.in2.size.minimum_size=}, {self.in2.size.maximum_size=}, {self.in2.size.fixed_size=}.'
)

def create_model(self, model) -> TransmissionModel:
self._plausibility_checks()
self.submodel = TransmissionModel(model, self)
return self.submodel

Expand All @@ -746,6 +631,43 @@ def __init__(self, model: FlowSystemModel, element: Transmission):

super().__init__(model, element)

def _validate(self):
"""Validate Transmission configuration for modeling."""
super()._validate()

# Check bus connections for bidirectional transmission
if self.element.in2 is not None:
if self.element.in2.bus != self.element.out1.bus:
raise ValueError(
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:
if self.element.out2.bus != self.element.in1.bus:
raise ValueError(
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=}.'
)

def _do_modeling(self):
"""Create variables, constraints, and nested submodels"""
super()._do_modeling()
Expand Down Expand Up @@ -787,6 +709,46 @@ def __init__(self, model: FlowSystemModel, element: LinearConverter):
self.piecewise_conversion: PiecewiseConversion | None = None
super().__init__(model, element)

def _validate(self):
"""Validate LinearConverter configuration for modeling."""
super()._validate()

# Check that either conversion_factors or piecewise_conversion is defined
if not self.element.conversion_factors and not self.element.piecewise_conversion:
raise PlausibilityError('Either conversion_factors or piecewise_conversion must be defined!')

# Check mutual exclusivity
if self.element.conversion_factors and self.element.piecewise_conversion:
raise PlausibilityError('Only one of conversion_factors or piecewise_conversion can be defined, not both!')

# Validate conversion_factors
if self.element.conversion_factors:
# Check degrees of freedom
if self.element.degrees_of_freedom <= 0:
raise PlausibilityError(
f'Too Many conversion_factors_specified. Care that you use less conversion_factors '
f'then inputs + outputs!! With {len(self.element.inputs + self.element.outputs)} inputs and outputs, '
f'use not more than {len(self.element.inputs + self.element.outputs) - 1} conversion_factors!'
)

# Validate flow names in conversion_factors
for conversion_factor in self.element.conversion_factors:
for flow in conversion_factor:
if flow not in self.element.flows:
raise PlausibilityError(
f'{self.element.label}: Flow {flow} in conversion_factors is not in inputs/outputs'
)

# Warn about piecewise_conversion with variable InvestParameters (modeling interaction)
if self.element.piecewise_conversion:
for flow in self.element.flows.values():
if isinstance(flow.size, InvestParameters) and flow.size.fixed_size is None:
logger.warning(
f'Using a Flow with variable size (InvestParameters without fixed_size) '
f'and a piecewise_conversion in {self.element.label_full} is uncommon. Please verify intent '
f'({flow.label_full}).'
)

def _do_modeling(self):
"""Create variables, constraints, and nested submodels"""
super()._do_modeling()
Expand Down Expand Up @@ -836,6 +798,63 @@ class StorageModel(ComponentModel):
def __init__(self, model: FlowSystemModel, element: Storage):
super().__init__(model, element)

def _validate(self):
"""Validate Storage configuration for modeling."""
super()._validate()

# Validate initial_charge_state string value
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=}.'
)

Comment on lines 801 to 857
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix initial-capacity bounds and .any() usage in StorageModel._validate

There are two intertwined issues in the storage validation:

  1. 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)
  1. 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.

def _do_modeling(self):
"""Create variables, constraints, and nested submodels"""
super()._do_modeling()
Expand Down
Loading