Feature/types direct #465
Feature/types direct #465
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR refactors flixopt's type system, introducing dimension-aware type aliases (Numeric_TPS, Numeric_PS, Numeric_S, Bool_TPS/PS/S, Effect_TPS/PS/S) in a new types.py module while removing legacy type aliases from core.py. Type annotations across 13 modules are updated to use the new system, with TimeSeriesData added as a public class. No functional behavior changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
flixopt/modeling.py (2)
33-41: Empty/None handling bug in to_binary
- values.size == 0 path accesses values.item(), which raises on empty.
- Function can't handle None (used by compute_previous_states).
Fix to gracefully handle None/empty.
- if values.size == 0: - return xr.DataArray(0) if values.item() < epsilon else xr.DataArray(1) + if values is None: + return xr.DataArray(0) + if getattr(values, "size", 0) == 0: + return xr.DataArray(0)
145-147: compute_previous_states should accept NoneMake signature and implementation robust to None by delegating to fixed to_binary.
- def compute_previous_states(previous_values: xr.DataArray | None, epsilon: float | None = None) -> xr.DataArray: - return ModelingUtilitiesAbstract.to_binary(values=previous_values, epsilon=epsilon, dims='time') + def compute_previous_states(previous_values: xr.DataArray | None, epsilon: float | None = None) -> xr.DataArray: + return ModelingUtilitiesAbstract.to_binary(values=previous_values, epsilon=epsilon, dims='time')(After applying the to_binary fix, this works without further changes.)
flixopt/components.py (1)
22-27: ImportNumeric_PSandNumeric_TPSat module scope to resolve F821 errorsThe review comment is accurate. These type aliases are used in runtime annotations (function parameters and attributes with default values) throughout the file, but are not imported at module scope. Python requires these names to be available at runtime, causing F821 errors when Ruff checks.
Apply the suggested import:
from .modeling import BoundingPatterns from .structure import FlowSystemModel, register_class_for_io +from .types import Numeric_PS, Numeric_TPSThis resolves all F821 errors across lines 390-400 (function parameters) and lines 417-429 (attribute annotations).
🧹 Nitpick comments (13)
flixopt/calculation.py (1)
106-106: Type hint for main_results is too narrow vs actual valuesEntries like Effects/Invest-Decisions hold arrays/lists after rounding, not only int|float. Recommend widening to Mapping[str, Any] (or a TypedDict later).
- def main_results(self) -> dict[str, int | float | dict]: + def main_results(self) -> dict[str, Any]:flixopt/features.py (1)
17-21: Outdated TYPE_CHECKING imports; align with new type systemScalar/TemporalData were removed from core. Import Scalar from flixopt.types and switch previous_states annotations to xr.DataArray.
@@ -if TYPE_CHECKING: - from .core import FlowSystemDimensions, Scalar, TemporalData +if TYPE_CHECKING: + from .core import FlowSystemDimensions + from .types import Scalar + import xarray as xrAdditionally, update OnOffModel.init:
- previous_states: TemporalData | None, + previous_states: xr.DataArray | None,Based on learnings.
flixopt/types.py (1)
18-33: Docstring uses undefined aliases (Numeric_T,Numeric_TS)The examples mention Numeric_T / Numeric_TS, but only *_TPS, *_PS, *_S are defined. Update docs to avoid confusion.
@@Repeat similarly where applicable. Based on learnings.
flixopt/flow_system.py (1)
35-36: TYPE_CHECKING imports reference non-existent aliasesBoolData/EffectData/NumericData/Period/Scenario/Time are not defined in flixopt.types. Import the new public aliases instead.
- from .types import BoolData, EffectData, NumericData, Period, Scenario, Time + from .types import Bool_TPS, Effect_TPS, Numeric_PS, Numeric_TPSThis keeps runtime clean while aiding type checkers. Based on learnings.
flixopt/modeling.py (2)
249-249: Return type annotation mismatch (dicts vs annotated tuple)consecutive_duration_tracking returns (variables_dict, constraints_dict) but is annotated as (linopy.Variable, tuple[...]). Align the type hint.
- ) -> tuple[linopy.Variable, tuple[linopy.Constraint, linopy.Constraint, linopy.Constraint]]: + ) -> tuple[dict[str, linopy.Variable], dict[str, linopy.Constraint]]:
200-208: bounds type overly strictbounds commonly passed as scalars/np.inf; widen the hint to accept float|int|xr.DataArray.
- bounds: tuple[xr.DataArray, xr.DataArray] = None, + bounds: tuple[int | float | xr.DataArray, int | float | xr.DataArray] = None,flixopt/components.py (2)
919-921: Remove stale TemporalData alias in annotationThe alias was dropped; use xr.DataArray.
- def _absolute_charge_state_bounds(self) -> tuple[TemporalData, TemporalData]: + def _absolute_charge_state_bounds(self) -> tuple[xr.DataArray, xr.DataArray]:
216-237: Conversion factors transform: PlausibilityError pathGood validation and transformation. Consider early-returning if ts is all zeros to avoid needless constraints, but optional.
flixopt/effects.py (2)
45-75: Docstring types for cross-effect shares are staleThe
Effectdocstring still refers toTemporalEffectsUserandPeriodicEffectsUserforshare_from_temporal/share_from_periodic, while the public signature now exposesEffect_TPS/Effect_PSand the rest of the library has moved to these aliases.Update the docstring type descriptions for these parameters to match the new aliases and their intended shapes.
493-531: Consider makingeffect_values_userexplicitly nullable in the type annotation
create_effect_values_dictbehaves like:
effect_values_user is None→Nonedict→ label-normalized dict- scalar/series → dict keyed by
standard_effect.labelThe signature now declares
effect_values_user: Effect_TPS, but the implementation still acceptsNone. UnlessEffect_TPSitself includesNone, it would be clearer (and easier for static checkers) to annotate this asEffect_TPS | None.Return typing to
dict[str, Numeric_TPS] | Nonematches the implementation.flixopt/elements.py (1)
718-747: Bounds properties may return scalars despitexr.DataArrayannotation
relative_flow_rate_boundsandabsolute_flow_rate_boundsare annotated to returntuple[xr.DataArray, xr.DataArray], but:
relative_flow_rate_boundscan return rawrelative_minimum/relative_maximumbeforetransform_data, which may be plain scalars.absolute_flow_rate_boundscan returnlb = 0when there is no investment or on/off, so the lower bound is a scalar.Runtime behavior is fine because the modeling code accepts scalars and arrays, but the annotations are slightly stricter than what is returned. Consider either:
- Relaxing the return type to
tuple[Numeric_TPS, Numeric_TPS], or- Wrapping scalar bounds into
xr.DataArraybefore returning.flixopt/linear_converters.py (2)
77-105: Converter constructors are correctly retargeted toNumeric_TPSBoiler, Power2Heat, HeatPump, CoolingTower, and CHP now annotate their efficiency-like parameters (
eta,COP,specific_electricity_demand,eta_th,eta_el) asNumeric_TPS, which matches the “per timestep/period/scenario” semantics and existing usage viaconversion_factors.One note: only the property setters (
eta,COP,specific_electricity_demand, etc.) callcheck_bounds. The initial values passed into__init__for Boiler, Power2Heat, and CHP are not checked unless the property is reassigned later. If you want bounds validation on construction as well, consider calling the respective setter from__init__.Also applies to: 164-193, 251-280, 340-371, 438-483
552-590: Validate COP > 1 before computingCOP / (COP - 1)in HeatPumpWithSource
HeatPumpWithSource.__init__currently buildsconversion_factorsusingCOP / (COP - 1)and only afterwards checksself.COPfor<= 1. For scalarCOP == 1, this will raise aZeroDivisionErrorbefore your explicit validation runs.Consider swapping the order:
- Validate
COP(orvalue) withcheck_bounds/np.any(...) <= 1.- Only then compute the
conversion_factorsinvolvingCOP / (COP - 1).This keeps the error mode predictable (your explicit
ValueError) even for the boundary caseCOP == 1.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
flixopt/__init__.py(1 hunks)flixopt/calculation.py(2 hunks)flixopt/components.py(7 hunks)flixopt/core.py(0 hunks)flixopt/effects.py(4 hunks)flixopt/elements.py(6 hunks)flixopt/features.py(2 hunks)flixopt/flow_system.py(5 hunks)flixopt/interface.py(9 hunks)flixopt/linear_converters.py(8 hunks)flixopt/modeling.py(7 hunks)flixopt/types.py(1 hunks)
💤 Files with no reviewable changes (1)
- flixopt/core.py
🧰 Additional context used
🧬 Code graph analysis (5)
flixopt/flow_system.py (2)
flixopt/effects.py (2)
Effect(32-403)EffectCollection(455-639)flixopt/structure.py (1)
weights(224-234)
flixopt/components.py (1)
flixopt/interface.py (1)
InvestParameters(671-1079)
flixopt/calculation.py (1)
flixopt/core.py (3)
DataConverter(144-561)TimeSeriesData(33-141)drop_constant_arrays(591-621)
flixopt/linear_converters.py (3)
flixopt/core.py (1)
TimeSeriesData(33-141)flixopt/elements.py (1)
Flow(285-551)flixopt/interface.py (1)
OnOffParameters(1083-1346)
flixopt/elements.py (1)
flixopt/interface.py (2)
InvestParameters(671-1079)OnOffParameters(1083-1346)
🪛 GitHub Actions: Python Package CI/CD
flixopt/__init__.py
[error] 14-14: Ruff linting error: I001 Import block is un-sorted or un-formatted
🪛 GitHub Check: lint
flixopt/components.py
[failure] 398-398: Ruff (F821)
flixopt/components.py:398:21: F821 Undefined name Numeric_TPS
[failure] 397-397: Ruff (F821)
flixopt/components.py:397:46: F821 Undefined name Numeric_PS
[failure] 396-396: Ruff (F821)
flixopt/components.py:396:46: F821 Undefined name Numeric_PS
[failure] 395-395: Ruff (F821)
flixopt/components.py:395:37: F821 Undefined name Numeric_PS
[failure] 394-394: Ruff (F821)
flixopt/components.py:394:37: F821 Undefined name Numeric_PS
[failure] 393-393: Ruff (F821)
flixopt/components.py:393:31: F821 Undefined name Numeric_PS
[failure] 392-392: Ruff (F821)
flixopt/components.py:392:40: F821 Undefined name Numeric_TPS
[failure] 391-391: Ruff (F821)
flixopt/components.py:391:40: F821 Undefined name Numeric_TPS
[failure] 390-390: Ruff (F821)
flixopt/components.py:390:33: F821 Undefined name Numeric_PS
🔇 Additional comments (12)
flixopt/features.py (1)
521-523: Good move to Numeric_PS for total boundsSwitching total_min/total_max to Numeric_PS matches PS dims. No concerns.
flixopt/flow_system.py (1)
523-571: fit_to_model_coords/fit_effects_to_model_coords typing alignmentSignatures now use Numeric_TPS/Bool_TPS/Effect_TPS; ensure any external references (docs, interface) are updated accordingly to prevent stale guidance.
Would you like me to scan the repo for stale references and open a follow-up PR?
flixopt/effects.py (1)
182-205: Effect constructor wiring to new alias types looks consistentThe new
Effect.__init__parameters and internal attributes forshare_from_temporal,share_from_periodic, and the various numeric bounds correctly use theEffect_TPS/Effect_PSandNumeric_TPS/Numeric_PSaliases, withNonemapped to{}for the share dicts.This keeps runtime behavior the same (no shares when falsy) while aligning with the new type system.
flixopt/elements.py (4)
16-27: Type wiring forPlausibilityErrorand new aliases looks goodImporting
PlausibilityErrorfrom.coreand the full set ofNumeric_*,Bool_*,Effect_*, andScalaraliases underTYPE_CHECKINGmatches how this file uses them (Flow, Bus, FlowModel). No issues here.
228-247: Bus excess penalty type aligns with model-coordinate handlingChanging
excess_penalty_per_flow_hourtoNumeric_TPS | Noneand passing it throughFlowSystem.fit_to_model_coordsintransform_datais consistent with how other TPS-like quantities are handled. The plausibility check usingnp.equal(..., 0)still works for scalars and array-like inputs.No further changes needed here.
418-447: Flow constructor updates are consistent with the new type aliasesThe
Flow.__init__signature and attribute assignments now use:
size: Numeric_PS | InvestParametersfixed_relative_profile, relative_minimum, relative_maximum: Numeric_TPSeffects_per_flow_hour: Effect_TPS | Numeric_TPS | None- Total-hours and load-factor bounds:
Numeric_PS | NoneDefaults (
CONFIG.Modeling.bigforsize,{}wheneffects_per_flow_hourisNone) preserve existing behavior while aligning with the new alias vocabulary and with downstreamtransform_dataand_create_shareslogic.Looks consistent with the rest of the refactor.
768-781:previous_statestyping matches downstream usageChanging
FlowModel.previous_statesto returnxr.DataArray | Nonematches whatModelingUtilitiesAbstract.to_binaryproduces and howOnOffModelis invoked withprevious_states=self.previous_states. The handling of scalar vs. list previous flow rates is unchanged.No issues here.
flixopt/linear_converters.py (1)
592-624:check_boundscorrectly unwraps TimeSeriesData but relies on broader Numeric_TPS semantics
check_boundsnow takesNumeric_TPSforvalue,lower_bound, andupper_boundand unpacksTimeSeriesDatato raw.databefore applyingnp.all(value > lower_bound)/< upper_boundchecks and logging warnings.Given that call sites always pass scalar bounds (
0or1/20), this remains safe for scalars and array-likevalue(including xarray data). No functional issues; the type change just makes the intent clearer.flixopt/interface.py (4)
75-84: Piece now aligned with TPS-style numeric dataUpdating
Piece.__init__tostart: Numeric_TPS, end: Numeric_TPSmatches the documented intent (scalar or time-varying boundaries) and howtransform_datapasses these throughfit_to_model_coordswith appropriate dims.The internal
has_time_dimflag continues to control whether period/scenario dims are applied, so behavior is preserved.
874-925: InvestParameters alias usage and new helpers are consistent
fixed_size,minimum_size,maximum_sizenow typed asNumeric_PS | Noneand normalized to numeric defaults (epsilon/big) when None.- Effect-related arguments use
Effect_PS | Numeric_PS | Noneand are stored as{}when None, which works withfit_effects_to_model_coords.- Deprecated properties
fix_effects,specific_effects, anddivest_effectsnow correctly advertiseEffect_PS | Numeric_PS, matching the underlying attributes.- New
minimum_or_fixed_size/maximum_or_fixed_sizehelpers returningNumeric_PSare used fromFlowModel.absolute_flow_rate_boundsandBoundingPatterns.scaled_bounds_with_state, giving a clean API for mandatory vs. optional investments.This wiring looks coherent with downstream usage.
Also applies to: 1002-1047
926-985: Transform and linked-periods logic matches newNumeric_PSsemantics
transform_datanow:
- Fits
minimum_size/maximum_size/linked_periods/fixed_sizeto['period', 'scenario']viafit_to_model_coords.- Converts tuple
(first_period, last_period)into a period-indexedxr.DataArraythroughcompute_linked_periodsbefore fitting.This is consistent with treating these parameters as
Numeric_PSand with howFlowModellater consumes them.
1265-1288: OnOffParameters correctly moved toEffect_TPS/Numeric_*aliasesThe
OnOffParametersconstructor now uses:
effects_per_switch_on/effects_per_running_hour:Effect_TPS | Numeric_TPS | None, defaulting to{}.- Time-based constraints:
consecutive_*_hours_*: Numeric_TPS | None.- Aggregate constraints:
on_hours_total_min/maxandswitch_on_total_maxstored asNumeric_PS.
transform_datafits all of these viafit_effects_to_model_coordsorfit_to_model_coords(with['period', 'scenario']where appropriate), and theuse_*properties still work by checking forNone.The updated typing is consistent with how
FlowModelandOnOffModelconsume these parameters.Also applies to: 1290-1317
| if TYPE_CHECKING: | ||
| from collections.abc import Iterator | ||
|
|
||
| from .flow_system import FlowSystem | ||
| from .types import EffectData, NumericData, Period, Scalar, Scenario, Time | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Import the new public type aliases under TYPE_CHECKING
This module uses Effect_TPS, Effect_PS, Numeric_TPS, and Numeric_PS in annotations (e.g., Effect.__init__, EffectCollection.create_effect_values_dict) but the TYPE_CHECKING import only brings in EffectData, NumericData, Period, Scalar, Scenario, Time. For static type checking tools, these aliases will appear undefined.
Extend the .types import here to include the new aliases that are referenced in this file.
🤖 Prompt for AI Agents
In flixopt/effects.py around lines 22 to 27, the TYPE_CHECKING import from
.types is missing the public type aliases Effect_TPS, Effect_PS, Numeric_TPS,
and Numeric_PS that are used elsewhere in this module; update the TYPE_CHECKING
block to import these four aliases from .types so static type checkers see them
and annotations resolve correctly.
| if TYPE_CHECKING: | ||
| from .elements import Flow | ||
| from .interface import OnOffParameters | ||
| from .types import NumericData, Period, Scenario, Time | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add Numeric_TPS to TYPE_CHECKING imports
This file uses Numeric_TPS in multiple annotations (converter constructor parameters and check_bounds), but the TYPE_CHECKING import from .types only brings in NumericData, Period, Scenario, Time.
For static typing, import Numeric_TPS here as well to keep annotations resolvable.
🤖 Prompt for AI Agents
In flixopt/linear_converters.py around lines 16 to 20, the TYPE_CHECKING block
imports NumericData, Period, Scenario, Time from .types but omits Numeric_TPS
which is referenced in annotations; update the TYPE_CHECKING imports to include
Numeric_TPS from .types so static type checkers can resolve annotations used in
converter constructors and check_bounds.
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)
flixopt/linear_converters.py (1)
552-589: Validate COP before constructing HeatPumpWithSource conversion factors to avoid division by zero
HeatPumpWithSource.__init__definesconversion_factors=[{P_el.label: COP, Q_th.label: 1}, {Q_ab.label: COP / (COP - 1), Q_th.label: 1}]before checkingself.COPand raising aValueErrorwhen any COP ≤ 1. If a user passesCOP <= 1(especiallyCOP == 1), this can computeCOP / (COP - 1)with a zero or negative denominator, leading to aZeroDivisionErroror infinities before your explicit validation runs.Consider validating COP strictly > 1 up front (on the raw
COPparameter) and only then constructing the two conversion factor dicts. For example:@@ - def __init__( - self, - label: str, - COP: Numeric_TPS, - P_el: Flow, - Q_ab: Flow, - Q_th: Flow, - on_off_parameters: OnOffParameters | None = None, - meta_data: dict | None = None, - ): - super().__init__( - label, - inputs=[P_el, Q_ab], - outputs=[Q_th], - conversion_factors=[{P_el.label: COP, Q_th.label: 1}, {Q_ab.label: COP / (COP - 1), Q_th.label: 1}], - on_off_parameters=on_off_parameters, - meta_data=meta_data, - ) + def __init__( + self, + label: str, + COP: Numeric_TPS, + P_el: Flow, + Q_ab: Flow, + Q_th: Flow, + on_off_parameters: OnOffParameters | None = None, + meta_data: dict | None = None, + ): + # Validate COP strictly > 1 before using it in denominators + if np.any(np.asarray(COP) <= 1): + raise ValueError(f'{label}.COP must be strictly > 1 for HeatPumpWithSource.') + + conv_el = {P_el.label: COP, Q_th.label: 1} + conv_src = {Q_ab.label: COP / (COP - 1), Q_th.label: 1} + + super().__init__( + label, + inputs=[P_el, Q_ab], + outputs=[Q_th], + conversion_factors=[conv_el, conv_src], + on_off_parameters=on_off_parameters, + meta_data=meta_data, + ) @@ - self.P_el = P_el - self.Q_ab = Q_ab - self.Q_th = Q_th - - if np.any(np.asarray(self.COP) <= 1): - raise ValueError(f'{self.label_full}.COP must be strictly > 1 for HeatPumpWithSource.') + self.P_el = P_el + self.Q_ab = Q_ab + self.Q_th = Q_thYou can keep the stricter check in the setter as well; the key point is to avoid constructing
COP/(COP-1)with invalid COP values in the constructor.
🧹 Nitpick comments (2)
flixopt/flow_system.py (1)
155-165: Broadenfit_to_model_coordstyping to match its actual usages
FlowSystem.__init__now takesweights: Numeric_PS | None, and callers likeFlowSystemModel.weightsalso pass plain scalars (1) and other PS-only quantities intofit_to_model_coords, but the helper is annotated as accepting onlyNumeric_TPS | Bool_TPS | None. Runtime-wise this works becauseDataConverter.to_dataarrayhandles scalars and PS-shaped inputs, yet static type checkers will flag these call sites unlessNumeric_PS/Scalarare included in (or aliased to)Numeric_TPS.Consider widening the signature (and any related docstring) to reflect the real surface, e.g. by explicitly allowing
Numeric_PSand scalar inputs, so the helper’s annotation matches how it is used throughout components, effects, and the weights logic.Also applies to: 523-562
flixopt/effects.py (1)
53-59: Docstring still references deprecated type names for cross-effect sharesThe
Effectdocstring forshare_from_temporal/share_from_periodicstill refers toTemporalEffectsUserandPeriodicEffectsUser, while the public surface and helpers now useEffect_TPS/Effect_PS(and Numeric_* aliases). Updating these type names in the docstring would avoid confusion for users reading the docs alongside the new type system.Also applies to: 68-74
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
flixopt/__init__.py(1 hunks)flixopt/components.py(8 hunks)flixopt/effects.py(4 hunks)flixopt/elements.py(6 hunks)flixopt/features.py(3 hunks)flixopt/flow_system.py(5 hunks)flixopt/interface.py(9 hunks)flixopt/linear_converters.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- flixopt/features.py
🧰 Additional context used
🧬 Code graph analysis (4)
flixopt/linear_converters.py (2)
flixopt/core.py (1)
TimeSeriesData(33-141)flixopt/interface.py (1)
OnOffParameters(1083-1344)
flixopt/components.py (2)
flixopt/core.py (1)
PlausibilityError(21-24)flixopt/interface.py (1)
InvestParameters(671-1079)
flixopt/elements.py (2)
flixopt/core.py (1)
PlausibilityError(21-24)flixopt/interface.py (2)
InvestParameters(671-1079)OnOffParameters(1083-1344)
flixopt/flow_system.py (2)
flixopt/effects.py (2)
Effect(32-399)EffectCollection(451-633)flixopt/structure.py (1)
weights(224-234)
🔇 Additional comments (20)
flixopt/__init__.py (1)
44-56: Top-level export of new dimension-aware type aliases looks goodRe-exporting
Bool_*,Effect_*,Numeric_*, andScalarfromflixopt.typesvia__init__cleanly exposes the new type system at the package level, and the multi-line import block is alphabetically ordered and linter-friendly.flixopt/flow_system.py (2)
25-25: Imports forEffectCollectionand TPS aliases are consistentBringing in
EffectCollectionand the TPS/PS type aliases underTYPE_CHECKINGmatches downstream usage (effects container and the new type system) without adding runtime dependencies, so this part looks correct.Also applies to: 35-36
563-586:fit_effects_to_model_coordstype surface aligns withEffectCollectionutilitiesThe updated signature
effect_values: Effect_TPS | Numeric_TPS | None -> Effect_TPS | NonematchesEffectCollection.create_effect_values_dictand the usage patterns inEffect.transform_dataandOnOffParameters.transform_data, and the implementation correctly normalizes scalars/dicts to a label→DataArray mapping.flixopt/components.py (4)
15-27: Core/type imports are now consistent with downstream usageImporting
PlausibilityErrorat runtime andNumeric_PS/Numeric_TPSunderTYPE_CHECKINGmatches the exceptions raised inLinearConverter._plausibility_checksand the new annotations without changing behavior.
167-237:LinearConverter.conversion_factorsannotation matches how factors are transformedTyping
conversion_factorsaslist[dict[str, Numeric_TPS]] | Nonealigns with_transform_conversion_factors, which passes each value throughFlowSystem.fit_to_model_coordsintoxr.DataArray. As long asNumeric_TPScovers the allowed scalar/time-series inputs, this keeps the public API expressive while matching internal handling.
385-405: Storage numeric/temporal typing and absolute bounds computation look coherentThe switch to
Numeric_PS/Numeric_TPSforStorageparameters and attributes fits the transform pipeline: temporal bounds (relative_*) are mapped with full TPS coords, period/scenario totals use PS dims, and_absolute_charge_state_boundsnow correctly returnsxr.DataArraylower/upper bounds combining relative profiles with either fixed capacity arrays or InvestParameters’ min/max sizes.This dovetails with
StorageModel._do_modeling’s variable creation and charge-state constraints without changing the underlying math.Also applies to: 414-430, 438-480, 920-931
660-691: Transmission loss parameters updated to Numeric_TPS without behavior changeAnnotating
relative_lossesandabsolute_lossesasNumeric_TPS | Noneand continuing to route them throughfit_to_model_coords(and thenp.any(element.absolute_losses != 0)check inTransmissionModel) matches the previous semantics while aligning with the new type system.Also applies to: 724-728
flixopt/effects.py (2)
22-27: Effect type annotations and transform logic align with the new TPS/PS type systemImporting
Effect_*/Numeric_*aliases underTYPE_CHECKING, annotatingshare_from_*and the various min/max fields with these types, and routing everything throughfit_effects_to_model_coords/fit_to_model_coords(with explicit dims) keeps the runtime behavior while making the dimensional expectations explicit. Defaultingshare_from_temporal/share_from_periodicto{}when None also simplifies downstream handling inEffectCollection.Also applies to: 174-202, 352-390
489-525:EffectCollection.create_effect_values_dictsignature matches FlowSystem usageThe updated signature
effect_values_user: Numeric_TPS | Effect_TPS | None -> Effect_TPS | Nonematches howFlowSystem.fit_effects_to_model_coordsandOnOffParameterssupply scalars, numeric TPS data, or effect→value dicts. The behavior of mapping scalars/None-labels viastandard_effectand warning onEffectobjects remains intact and consistent with the docstring.flixopt/linear_converters.py (1)
16-20: Numeric_TPS annotations andcheck_boundsintegration are consistentImporting
TimeSeriesDataat runtime andNumeric_TPSunderTYPE_CHECKING, then annotating efficiency/COP/specific_electricity_demand parameters asNumeric_TPS, matches how these values are subsequently handled (either as scalars or time series converted via the FlowSystem helpers). The updatedcheck_boundshelper correctly unwrapsTimeSeriesDatato raw numpy data and continues to log warnings for values outside the (exclusive) bounds without altering runtime behavior.Also applies to: 77-93, 164-193, 251-280, 340-371, 439-466, 592-598
flixopt/elements.py (5)
16-16: LGTM! Import changes align with the new type system.The TYPE_CHECKING imports now reference the new dimension-aware type aliases from
.types, and PlausibilityError is correctly imported from.core. These changes are consistent with the PR's objective to update the type system.Also applies to: 26-37
242-242: LGTM! Parameter type updated correctly.The
excess_penalty_per_flow_hourparameter type is nowNumeric_TPS | None, which correctly represents time-period-scenario-varying numeric data in the new type system.
433-443: LGTM! Flow parameter types updated systematically.All parameter types have been updated to use the new type aliases:
Numeric_PSfor period-scenario varying parameters (size bounds, load factors)Numeric_TPSfor time-period-scenario varying parameters (profiles, relative bounds)Effect_TPSfor effects that vary over timeThe type changes correctly reflect the dimensionality of each parameter.
730-733: LGTM! Return types explicitly specify xr.DataArray.The return types for
relative_flow_rate_boundsandabsolute_flow_rate_boundsnow explicitly usetuple[xr.DataArray, xr.DataArray], making the API more explicit and consistent with xarray usage throughout the type system.Also applies to: 736-757
779-792: LGTM! Return type correctly specifies xr.DataArray.The
previous_statesproperty return type is nowxr.DataArray | None, which accurately reflects the implementation that converts previous flow rate data to a binary DataArray.flixopt/interface.py (5)
23-23: LGTM! TYPE_CHECKING imports now include required type aliases.The TYPE_CHECKING block now imports
Effect_PS,Effect_TPS,Numeric_PS, andNumeric_TPSfrom.types, which are used throughout this module. This addresses the concern raised in the previous review about missing alias types.
75-75: LGTM! Piece parameter types updated correctly.The
startandendparameters now useNumeric_TPS, which correctly represents time-period-scenario-varying boundaries for piecewise linear segments.
876-884: LGTM! InvestParameters types updated systematically.The constructor parameters and attribute assignments have been updated to use the new type system:
- Size parameters use
Numeric_PSfor period-scenario varying data- Effect parameters use
Effect_PS | Numeric_PSto support both effect dictionaries and simple numeric values- Default values (empty dicts for effects) are appropriate
The implementation correctly handles the new types.
Also applies to: 914-920
1002-1047: LGTM! Deprecated property return types aligned with new system.The deprecated properties (
fix_effects,specific_effects,divest_effects,minimum_or_fixed_size,maximum_or_fixed_size) now have return types that match the new type system, correctly returningEffect_PS | Numeric_PSorNumeric_PSas appropriate.
1277-1278: LGTM! Effect parameter handling is correct.The
effects_per_switch_onandeffects_per_running_hourparameters are correctly handled with defaults to empty dicts when None, and their types (Effect_TPS | Numeric_TPS | None) appropriately support both effect dictionaries and simple numeric values.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/linear_converters.py (1)
595-627: Now let me verify that the proposed fix is semantically correct and doesn't break anything:
check_boundscan fail for xarray.DataArray inputs; convert to NumPy arrays firstThe function accepts
Numeric_TPS(which includes xr.DataArray, pd.Series, and pd.DataFrame) but only unwrapsTimeSeriesDatato.data. Plainxr.DataArrayinputs are not unwrapped, so:if not np.all(value > lower_bound): ...will fail because
np.all()on a DataArray comparison returns a 0-D DataArray, not a bool, causing an "ambiguous truth value" error in the boolean context.To fix this, coerce all inputs to NumPy arrays:
+ value_arr = np.asarray(value) + lower_arr = np.asarray(lower_bound) + upper_arr = np.asarray(upper_bound) + + if not np.all(value_arr > lower_arr): logger.warning( f"'{element_label}.{parameter_label}' is equal or below the common lower bound {lower_bound}." - f' {parameter_label}.min={np.min(value)}; {parameter_label}={value}' + f' {parameter_label}.min={np.min(value_arr)}; {parameter_label}={value}' ) + if not np.all(value_arr < upper_arr): logger.warning( f"'{element_label}.{parameter_label}' exceeds or matches the common upper bound {upper_bound}." - f' {parameter_label}.max={np.max(value)}; {parameter_label}={value}' + f' {parameter_label}.max={np.max(value_arr)}; {parameter_label}={value}' )This maintains the warning semantics for all accepted types while avoiding truth value ambiguity.
♻️ Duplicate comments (1)
flixopt/interface.py (1)
1264-1286: AllowOnOffParametersattributes to be optional in type hints
on_hours_total_min/max,consecutive_*_hours_*, andswitch_on_total_maxare annotated asNumeric_PS/Numeric_TPS, but they are allowed to beNoneon construction and are explicitly tested withparam is not Nonein theuse_*properties and used as sentinels beforetransform_dataconverts them.For static clarity and to avoid confusing consumers of this API, it would be better to reflect this in the annotations, e.g.:
- self.on_hours_total_min: Numeric_PS = on_hours_total_min - self.on_hours_total_max: Numeric_PS = on_hours_total_max + self.on_hours_total_min: Numeric_PS | None = on_hours_total_min + self.on_hours_total_max: Numeric_PS | None = on_hours_total_max ... - self.consecutive_on_hours_min: Numeric_TPS = consecutive_on_hours_min + self.consecutive_on_hours_min: Numeric_TPS | None = consecutive_on_hours_min ... - self.switch_on_total_max: Numeric_PS = switch_on_total_max + self.switch_on_total_max: Numeric_PS | None = switch_on_total_max(or similarly for all four consecutive_* and both on_hours_total_* fields).
This keeps runtime behavior unchanged but makes the intent (
None= “no constraint”) explicit in the type system.
🧹 Nitpick comments (2)
flixopt/features.py (1)
514-542: Clarify unbounded*_per_hourhandling to avoid mixedNone/±inf semantics
ShareAllocationModel.__init__now acceptsmax_per_hour/min_per_hour: Numeric_TPS | Noneand stores them asnp.inf/-np.infwhenNone, but_do_modelingstill checksself._min_per_hour is None/self._max_per_hour is Nonebefore deciding whether to apply per‑timestep bounds. This works numerically (±inf × hours_per_step) but mixes two sentinel conventions and makes the “no bound” case harder to reason about.Consider keeping the internal attributes as
Nonewhen unbounded and deriving ±inf only where needed (or switching the checks to look fornp.isinf), so the behavior is explicit and less error‑prone.flixopt/effects.py (1)
489-525: Minor typing gap increate_effect_values_dictaroundNoneandEffectkeysThe new signature
effect_values_user: Numeric_TPS | Effect_TPS | None -> Effect_TPS | Nonematches the broader type system, but the implementation still deliberately accepts dict keys that areNoneorEffectinstances (handled viaget_effect_label). Static types don’t currently express that:
Effect_TPSis keyed bystr, but{None: value}and{Effect(): value}are treated specially at runtime.If you want stricter static checking, consider either:
- documenting these extra key forms explicitly, or
- widening the internal helper signature (e.g.
get_effect_label(eff: Effect | str | None)) so the intent is clearer to readers and type checkers.This is purely a clarity/typing nit, not a runtime issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
flixopt/core.py(2 hunks)flixopt/effects.py(4 hunks)flixopt/features.py(3 hunks)flixopt/flow_system.py(5 hunks)flixopt/interface.py(9 hunks)flixopt/io.py(2 hunks)flixopt/linear_converters.py(19 hunks)flixopt/types.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- flixopt/flow_system.py
🧰 Additional context used
🧬 Code graph analysis (2)
flixopt/features.py (2)
flixopt/interface.py (3)
InvestParameters(671-1079)OnOffParameters(1083-1344)Piecewise(87-228)flixopt/elements.py (2)
previous_states(779-792)previous_states(907-924)
flixopt/linear_converters.py (2)
flixopt/core.py (1)
TimeSeriesData(35-143)flixopt/interface.py (1)
OnOffParameters(1083-1344)
🪛 GitHub Actions: Python Package CI/CD
flixopt/types.py
[warning] 1-1: Ruff formatting would reformat this file as part of a pre-commit/CI formatting step. 2 files would be reformatted.
flixopt/linear_converters.py
[warning] 1-1: Ruff formatting would reformat this file as part of a pre-commit/CI formatting step. 2 files would be reformatted.
🔇 Additional comments (9)
flixopt/types.py (1)
1-109: Dimension-aware aliases look good; please run Ruff formatterThe
_Numeric/_Boolbases and theNumeric_*,Bool_*,Effect_*,Scalar, andNumericOrBoolaliases are coherent and match the documented “maximum dimensions” semantics; exporting them via__all__gives a clean, centralized typing surface for the rest of the codebase.CI reports Ruff would reformat this file; please run your formatting hook (e.g.
ruff format flixopt/types.py) before merging so the pipeline stays green.flixopt/io.py (1)
19-23: numeric_to_str_for_repr is now correctly aligned with the new type aliasesImporting
Numeric_TPSunderTYPE_CHECKINGand annotatingnumeric_to_str_for_repr.valuewith it removes the ad‑hoc union from this file and keeps IO utilities in sync with the centralized type system, without changing runtime behavior.Also applies to: 655-659
flixopt/features.py (1)
17-21: OnOffModel.previous_states typing is consistent with the new Numeric_TPS aliasesUsing
Numeric_TPS | Noneforprevious_statesand importing the alias underTYPE_CHECKINGmatches howprevious_statesare produced (xarray-based time series) and keeps this feature in step with the updated type system, without changing runtime behavior.Also applies to: 151-159
flixopt/core.py (1)
15-17: NumericOrBool import and annotation clean up DataConverter’s typingImporting
NumericOrBoolfrom.typesand annotatingDataConverter.to_dataarraywith it replaces the local, expanded union with a single canonical alias, matching the docstring’s supported types and simplifying maintenance. The logger relocation is benign.Also applies to: 392-395
flixopt/linear_converters.py (1)
12-21: Numeric_TPS/TimeSeriesData integration and conversion_factors setters look consistent
- Importing
TimeSeriesDataat runtime andNumeric_TPSunderTYPE_CHECKINGlets all converter classes accept the new dimension-aware numeric aliases while still unwrapping TimeSeriesData cleanly insidecheck_bounds.- Using
Numeric_TPSin the boiler, Power2Heat, HeatPump, CoolingTower, CHP, and HeatPumpWithSource constructors matches the new type system and the documented “time / period / scenario” semantics.- Initializing
conversion_factorsvia theeta/COP/specific_electricity_demandsetters (withconversion_factors=[]passed intosuper().__init__where needed) keeps the internal mapping in sync whenever those parameters are reassigned and avoids in-place mutation of nested structures.CI also reports Ruff would reformat this file; please run your formatter (e.g.
ruff format flixopt/linear_converters.py) so style checks pass.Also applies to: 77-85, 164-172, 251-271, 340-348, 436-446, 557-574
flixopt/effects.py (2)
22-27: TYPE_CHECKING imports now align with alias usageImporting
Effect_PS,Effect_TPS,Numeric_PS,Numeric_TPS, andScalarunderTYPE_CHECKINGmatches the annotations used throughout this module and should keep static type checkers happy without affecting runtime.
53-75: Effect constructor and docstring correctly migrated to new alias typesThe updated docstring and
Effect.__init__signature consistently useEffect_TPS/Effect_PSfor cross-effect shares andNumeric_PS/Numeric_TPSfor bounds, and the{}defaults forshare_from_temporal/share_from_periodickeep the rest of the code (loops over.items(), falsy checks) working as before.Also applies to: 182-201
flixopt/interface.py (2)
19-24: TYPE_CHECKING aliases andPiececonstructor look consistent with refactorBringing in
Effect_PS,Effect_TPS,Numeric_PS, andNumeric_TPSunderTYPE_CHECKINGmatches the annotations used later in this module, and updatingPiece.__init__tostart: Numeric_TPS, end: Numeric_TPSaligns with howtransform_datapasses these throughfit_to_model_coords(with time-dim optionally enabled viahas_time_dim).Also applies to: 75-83
874-925: InvestParameters effect/numeric typing and defaults are coherent with new type systemSwitching
fixed_size,minimum_size,maximum_size, and the variouseffects_of_*arguments toNumeric_PS/Effect_PS | Numeric_PSplus normalizingeffects_of_*to{}whenNonekeepstransform_datasimple (always passing a mapping intofit_effects_to_model_coords) and matches the updated deprecated properties (fix_effects,specific_effects,divest_effects) and theminimum_or_fixed_size/maximum_or_fixed_sizehelpers.No functional issues stand out; these changes look consistent with the alias-based API.
Also applies to: 1002-1047
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
flixopt/linear_converters.py (1)
471-488: Consider adding a brief comment explaining the setter initialization pattern.The conditional initialization in both
eta_thandeta_elsetters correctly handles either property being set first. While the implementation is sound, a brief inline comment explaining this dual-property initialization pattern would improve maintainability.For example:
@eta_th.setter def eta_th(self, value): check_bounds(value, 'eta_th', self.label_full, 0, 1) + # Initialize 2-element structure if needed (handles either eta_th or eta_el being set first) if len(self.conversion_factors) < 2: - # Initialize structure if not yet set self.conversion_factors = [{self.Q_fu.label: value, self.Q_th.label: 1}, {}] else: self.conversion_factors[0] = {self.Q_fu.label: value, self.Q_th.label: 1}flixopt/types.py (1)
82-93: Consider generalizing_Effectmapping to support non-dict mappings (optional)
_Effectis currently defined as_Numeric | dict[str, _Numeric], which is fine, but a bit restrictive if you ever want to pass other mapping types (e.g.Mapping[str, _Numeric],OrderedDict, custom mapping implementations).Not urgent, but if you expect non-
dictmappings in practice, it may be slightly more flexible (and more in line with standard typing conventions) to use a generic mapping type instead.from collections.abc import Mapping _Effect: TypeAlias = _Numeric | Mapping[str, _Numeric]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
flixopt/linear_converters.py(19 hunks)flixopt/types.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/linear_converters.py (3)
flixopt/core.py (1)
TimeSeriesData(35-143)flixopt/elements.py (1)
Flow(296-562)flixopt/interface.py (1)
OnOffParameters(1083-1344)
🔇 Additional comments (5)
flixopt/linear_converters.py (2)
16-20: Previous review comment has been addressed.The
Numeric_TPStype is now correctly imported in the TYPE_CHECKING block, resolving the issue raised in the previous review.
612-617: Correct handling of TimeSeriesData in bounds checking.The extraction of the
.dataattribute fromTimeSeriesDataobjects before comparison is appropriate, as it allows numpy comparison operations to work correctly with the underlying numeric data.flixopt/types.py (3)
1-39: Docstring clearly explains the new dimension-aware type modelThe documentation here is clear, concrete, and gives good examples of intended usage and broadcasting semantics. No changes needed from my side.
100-112: Export list looks consistent with defined public aliases
__all__cleanly exposes just the intended public surface (Numeric_*,Bool_*,Effect_*,Scalar,NumericOrBool) and keeps the internal bases (_Numeric,_Bool,_Effect) private. This matches the docstring narrative and seems like the right API boundary.
41-51: Review comment is valid and concerns are resolved — no changes neededVerification confirms the code is correct as-is:
- Project requires Python 3.10+ (
requires-python = ">=3.10"), soTypeAliasandX | Yunion syntax are natively supported without fallbacks.- numpy, pandas, and xarray are all listed as required dependencies, not optional, so the direct module-level imports in
flixopt/types.pyare appropriate.The review comment's closing statement—"If the intended support matrix is Python 3.10+ and all three libraries are mandatory dependencies, then this is fine as-is"—accurately describes the project's actual configuration. No adjustments or compatibility layers are required.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
flixopt/effects.py (1)
22-27: TYPE_CHECKING imports now include new type aliases.The TYPE_CHECKING block now imports
Effect_PS,Effect_TPS,Numeric_PS,Numeric_TPS, andScalaras flagged in the previous review. These aliases are used throughout the module in type annotations.
🧹 Nitpick comments (2)
flixopt/types.py (1)
82-92: Clarify Effect type docstrings.The docstrings state that Effect types "Can be single numeric value or dict mapping effect names to values," but the underlying
_Effecttype (line 50) is defined asdict[str, _Numeric]only. The "single numeric value" capability comes from union withNumeric_*types at usage sites (e.g.,Effect_TPS | Numeric_TPSin effects.py line 182).Update the docstrings to clarify that Effect types themselves are dicts, and single numeric values are supported through union with Numeric types in function signatures.
Apply this diff:
Effect_TPS: TypeAlias = _Effect -"""Time, Period, Scenario dimensions. For time-varying effects (costs, emissions). -Can be single numeric value or dict mapping effect names to values.""" +"""Time, Period, Scenario dimensions. Dict mapping effect names to values. +For time-varying effects (costs, emissions). Use `Effect_TPS | Numeric_TPS` to accept single values.""" Effect_PS: TypeAlias = _Effect -"""Period, Scenario dimensions. For period-specific effects (investment costs). -Can be single numeric value or dict mapping effect names to values.""" +"""Period, Scenario dimensions. Dict mapping effect names to values. +For period-specific effects (investment costs). Use `Effect_PS | Numeric_PS` to accept single values.""" Effect_S: TypeAlias = _Effect -"""Scenario dimension. For scenario-specific effects (carbon prices). -Can be single numeric value or dict mapping effect names to values.""" +"""Scenario dimension. Dict mapping effect names to values. +For scenario-specific effects (carbon prices). Use `Effect_S | Numeric_S` to accept single values."""flixopt/effects.py (1)
182-200: Consider the semantics of defaulting share parameters to empty dict.The
share_from_temporalandshare_from_periodicparameters acceptEffect_TPS | Numeric_TPSandEffect_PS | Numeric_PSunions respectively, meaning users can pass either a dict or a single numeric value. However, whenNoneis provided, both default to{}(empty dict).This creates an intermediate state where
self.share_from_temporalcould be either a dict or a numeric value untiltransform_data()normalizes it. The empty dict default is correct for the "no shares" case, but the mixed type handling between initialization and transformation could be clearer.The current implementation works because
fit_effects_to_model_coords()(line 358) handles both dict and numeric inputs, but consider documenting this transformation flow or validating the type earlier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
flixopt/effects.py(4 hunks)flixopt/types.py(1 hunks)
⏰ 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.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (3)
flixopt/types.py (1)
1-112: Type system design looks solid.The dimension-aware type system with suffix notation provides clear semantic meaning for parameters while maintaining runtime flexibility. The documentation is comprehensive and the use of TypeAlias provides good IDE support for type hints.
Note that the type aliases don't enforce dimension constraints at runtime—they're identical unions that rely on documentation and convention. This is acceptable for a gradual typing approach, but downstream code must validate dimensions when converting to DataArrays.
flixopt/effects.py (2)
489-524: Effect values dict creation handles both scalars and mappings correctly.The
create_effect_values_dictmethod properly converts scalar values to dicts using the standard effect, handles dict inputs with label resolution, and includes deprecation warnings for Effect object usage. The signatureNumeric_TPS | Effect_TPS | Nonecorrectly accepts both single values and mappings.
54-74: Docstrings updated with new type references.The parameter docstrings now reference the new type aliases (
Effect_TPS,Effect_PS,Numeric_TPS,Numeric_PS) and clarify dimension constraints. The documentation clearly explains that constraints likeminimum_temporalsum over time and vary by period/scenario, which aligns with the type signatures.
- Problem: Docstrings incorrectly stated Effect types "can be single numeric value" - Solution: Clarified that Effect types are dicts, and single values work through union types like Effect_TPS | Numeric_TPS 5. Documentation: Share parameter semantics (flixopt/effects.py:199-203) - Problem: Mixed handling of dict/numeric values between initialization and transformation wasn't documented - Solution: Added clarifying comments explaining that parameters accept unions, are stored as-is, and normalized later via transform_data() All changes maintain backward compatibility while fixing runtime errors and improving code clarity
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
flixopt/linear_converters.py (1)
436-489: Setter initialization logic is fragile but functional for current usage.The CHP class initializes
conversion_factorsas an empty list (line 451) and relies on setters to build the two-entry structure. The conditional logic in both setters handles initialization vs. update:
eta_th.settermanagesconversion_factors[0]eta_el.settermanagesconversion_factors[1]Since both setters are always called in
__init__(lines 459-460), the structure is correctly built. However, if someone later sets only one property independently, the other entry could remain as an empty dict{}, which might cause issues downstream.Consider consolidating the initialization to avoid partial states:
def __init__( self, label: str, eta_th: Numeric_TPS, eta_el: Numeric_TPS, Q_fu: Flow, P_el: Flow, Q_th: Flow, on_off_parameters: OnOffParameters | None = None, meta_data: dict | None = None, ): super().__init__( label, inputs=[Q_fu], outputs=[Q_th, P_el], - conversion_factors=[], + conversion_factors=[{}, {}], # Pre-initialize two-entry structure on_off_parameters=on_off_parameters, meta_data=meta_data, )This would make the setters simpler and more robust.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
flixopt/effects.py(4 hunks)flixopt/features.py(4 hunks)flixopt/interface.py(9 hunks)flixopt/linear_converters.py(20 hunks)flixopt/types.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- flixopt/types.py
🧰 Additional context used
🧬 Code graph analysis (2)
flixopt/features.py (3)
flixopt/interface.py (3)
InvestParameters(671-1079)OnOffParameters(1083-1344)Piecewise(87-228)flixopt/elements.py (5)
previous_states(779-792)previous_states(907-924)_do_modeling(571-600)_do_modeling(803-825)_do_modeling(849-896)flixopt/effects.py (2)
_do_modeling(411-448)_do_modeling(676-689)
flixopt/linear_converters.py (1)
flixopt/core.py (1)
TimeSeriesData(35-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (17)
flixopt/linear_converters.py (6)
16-19: Past review concern about Numeric_TPS import has been addressed.The TYPE_CHECKING block now correctly imports
Numeric_TPSfrom.types(line 19), which resolves the previous review comment. The runtime import ofTimeSeriesDatafrom.core(line 13) is also appropriate since it's used inisinstance()checks withincheck_bounds().
77-105: LGTM! Type update and setter logic are correct.The parameter type change to
Numeric_TPSaligns with the new type system. The setter correctly rebuildsconversion_factorsas a single-entry list with the fuel-to-thermal relationship, and bounds checking is properly applied.
164-193: LGTM! Consistent with the Boiler implementation.The
Power2Heatclass follows the same pattern asBoilerwith appropriate type updates and setter logic for single-output conversion.
557-593: LGTM! Cleaner setter implementation than CHP.The
HeatPumpWithSourcesetter atomically creates bothconversion_factorsentries (lines 589-592), avoiding the fragility of CHP's incremental approach. The strict COP > 1 validation (line 587) and the thermodynamic formulas are correct.
251-280: LGTM! Both classes correctly implement the new type system.
HeatPumpandCoolingTowerfollow clean setter patterns with proper type updates. The negative coefficient forP_elinCoolingTower(line 368) correctly models electricity as an input.Also applies to: 340-369
595-633: LGTM! Robust handling of various numeric types.The updated
check_boundsfunction correctly handles theNumeric_TPSunion type by:
- Converting
TimeSeriesDatainstances to their underlying.data(lines 612-617)- Converting all inputs to NumPy arrays for uniform comparison (lines 619-622)
- Using element-wise comparisons with
np.all()(lines 624, 629)The warning messages include both summary statistics (
min/max) and the original value representation for user clarity.flixopt/features.py (3)
17-20: LGTM! TYPE_CHECKING imports updated for new type system.The imports now reference the new type aliases (
Numeric_PS,Numeric_TPS) from.types, consistent with the PR's type system refactor.
151-159: LGTM! Parameter type updated correctly.The
previous_statesparameter type change toNumeric_TPS | Nonealigns with the new type system and is compatible with the xarray operations performed on it (e.g.,.isel()on line 207).
514-566: LGTM! Clean None-aware bounds handling.The
ShareAllocationModelcorrectly handles optional bounds by:
- Storing parameters as-is with
Noneallowed (lines 539-542)- Using explicit None checks with
-np.inf/np.inffallbacks when creating variables (lines 549-550, 560-561)- Properly converting per-hour bounds to per-timestep by multiplying with
hours_per_step(lines 560-561)This pattern provides clear, explicit control flow.
flixopt/effects.py (3)
22-26: Past review concern about missing type imports has been addressed.The TYPE_CHECKING block now includes all the required type aliases (
Effect_TPS,Effect_PS,Numeric_TPS,Numeric_PS) from.types(line 26), resolving the previous review comment about undefined type annotations.
174-232: LGTM! Parameter types and defaults are well-designed.The updated parameter types (
Effect_TPS | Numeric_TPS,Effect_PS | Numeric_PS) provide flexibility for both dict-based and scalar effect values. Defaulting share parameters to{}(lines 202-203) is a sensible choice, clearly indicating "no shares" while avoiding None checks throughout the codebase.
492-527: LGTM! Clean type-aware conversion logic.The
create_effect_values_dictfunction correctly handles the union input type (Numeric_TPS | Effect_TPS | None) by:
- Returning
NoneforNoneinput (line 523)- Mapping dict keys through label resolution (lines 525-526)
- Wrapping scalars with the standard effect label (line 527)
The implementation maintains backward compatibility while supporting the new type system.
flixopt/interface.py (5)
19-23: Past review concern about missing type imports has been addressed.The TYPE_CHECKING block now includes the necessary type aliases (
Effect_PS,Effect_TPS,Numeric_PS,Numeric_TPS) from.types(line 23), resolving the previous review comment.
75-78: LGTM! Simple type update.The
Piececlass parameters are correctly updated to useNumeric_TPS, consistent with the new type system.
874-924: LGTM! Constructor correctly handles new type system.The
InvestParametersconstructor properly:
- Updates parameter types to the new aliases (
Numeric_PS,Effect_PS | Numeric_PS)- Defaults effect parameters to
{}whenNone(lines 914-915, 918-920)- Uses
CONFIGconstants for size bounds when unspecified (lines 922-923)- Supports flexible
linked_periodsinput (lines 884, 924)
1001-1047: LGTM! Property return types correctly updated.All property return types have been updated to match the new type system (
Effect_PS | Numeric_PSfor effect-related properties,Numeric_PSfor size properties), maintaining consistency with the underlying attribute types.
1264-1286: LGTM! Constructor parameters correctly typed.The
OnOffParametersconstructor properly uses:
Effect_TPS | Numeric_TPSfor temporal effect parameters (lines 1266-1267)Numeric_PSfor period/scenario-level constraints (lines 1268-1269, 1274)Numeric_TPSfor time-varying constraints (lines 1270-1273)The parameter-to-attribute assignments with
{}defaults for effect dicts (lines 1277-1278) follow the established pattern from other Parameter classes.
Description
Update the type system in flixopt
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Refactor
Docs