Feature/simplify structure #464
Conversation
WalkthroughRemove FlowSystem parameters from element/model transform_data signatures; add per-element FlowSystem linkage via _set_flow_system/flow_system; centralize coordinate/effect fitting into _fit_coords/_fit_effect_coords; propagate FlowSystem to nested objects and validate system integrity before model creation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant FS as FlowSystem
participant Reg as Registry (_add_*)
participant Elem as Element / Interface
participant Fit as _fit_coords / _fit_effect_coords
User->>FS: connect_and_transform()
FS->>Reg: add_elements(...)
Reg->>Elem: _set_flow_system(FS)
Elem->>Elem: store self._flow_system
FS->>Elem: transform_data(name_prefix)
activate Elem
Elem->>Fit: _fit_coords(prefix, data) / _fit_effect_coords(prefix, values)
activate Fit
Fit->>FS: delegate to FlowSystem.fit_to_model_coords / fit_effects_to_model_coords
FS-->>Fit: fitted data
Fit-->>Elem: return fitted data
deactivate Fit
Elem-->>FS: transformed data stored
deactivate Elem
FS->>FS: _validate_system_integrity()
FS-->>User: ready for create_model / solve
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/structure.py (1)
257-290: transform_data API change is correct but breaking; update docs accordinglyChanging
Interface.transform_datato accept onlyname_prefixand rely on an internalflow_systemlink matches the PR’s direction, but it is a backwards-incompatible public API change. Also, the class docstring still mentionstransform_data(flow_system)and should be aligned with the new signature.
Please ensure user-facing docs / release notes call out this change and that any external callers no longer pass a FlowSystem argument.
🧹 Nitpick comments (3)
flixopt/flow_system.py (1)
647-649: System integrity validation is useful but currently limited to flows→busesCalling
_validate_system_integrity()before model construction gives a clearer error path for missing buses and is cheap. Right now it only checksflow.bus in self.buses; consider extending it later (e.g., component/flow consistency, duplicated labels) if you want this to be a central consistency gate.Also applies to: 782-804
flixopt/structure.py (1)
291-367: FlowSystem linkage helpers are sound; be aware of private-attribute caveat
_set_flow_systemplus theflow_systemproperty and_fit_coords/_fit_effect_coordsgive a clean way to access FlowSystem context from any Interface subclass and its nested children. Because_set_flow_systemskips attributes whose names start with_, any nested Interface stored only under a private attribute will not be linked; that’s fine today but worth keeping in mind if future parameter objects are attached via private fields. Initializing_flow_systeminElement.__init__completes the contract.Also applies to: 936-946
flixopt/interface.py (1)
932-949: Consider consistent argument passing style for_fit_effect_coords().The calls to
_fit_effect_coords()use keyword arguments here (lines 932-949), but Line 1296 inOnOffParameters.transform_datauses positional arguments. While both may work, consistent style improves maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
flixopt/components.py(3 hunks)flixopt/effects.py(1 hunks)flixopt/elements.py(3 hunks)flixopt/flow_system.py(3 hunks)flixopt/interface.py(7 hunks)flixopt/structure.py(2 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.11)
- GitHub Check: test (3.10)
🔇 Additional comments (15)
flixopt/flow_system.py (2)
597-608: Simplified connect_and_transform call path looks consistentUsing
element.transform_data()without a FlowSystem parameter is fine now that elements are linked via_set_flow_system;Component,Bus,Effect, and nested parameter classes all rely on theflow_systemproperty instead.
805-808: FlowSystem linkage via _set_flow_system is wired correctlyHooking
_set_flow_system(self)in_add_effects,_add_components, and_add_busesensures all Elements (and their nested Interface children) have a validflow_systemreference beforetransform_dataruns, which matches the new design.
Please double-check that all element creation paths (including any legacy helpers or deserialization utilities) go through these_add_*methods so nothing can be transformed without being linked first.Also applies to: 810-817, 819-824
flixopt/effects.py (1)
343-378: Effect.transform_data correctly uses the new helpers and dimsThe refactored
transform_datacleanly routes everything through_fit_coords/_fit_effect_coordswith sensible prefixes and dimensionality for temporal vs periodic data; the assignment back onto the instance matches downstream expectations inEffectCollectionModeland share handling.flixopt/elements.py (2)
103-110: Component/Flow transform_data wiring aligns with the new linkage model
Component.transform_datanow only prefixes component-level OnOffParameters and lets eachFlowhandle its own fit via_fit_coords/_fit_effect_coordsusinglabel_full, which is sufficient for uniqueness. This slightly changes the generated variable names compared to passing a component prefix intoFlow.transform_data, so it’s worth checking any tests or tooling that rely on exact DataArray names, but the optimization logic itself remains consistent.Also applies to: 471-496
244-248: Bus.transform_data correctly moves penalty fitting behind _fit_coordsTransforming
excess_penalty_per_flow_hourthrough_fit_coordswith a prefixed name is consistent with other elements and ensures the penalty series is aligned with the FlowSystem coordinates before modeling.flixopt/components.py (3)
215-223: LinearConverter.transform_data and conversion-factor normalization look correctDelegating to
Component.transform_data(prefix)and then convertingconversion_factorsvia_transform_conversion_factors()ensures all coefficients are aligned to model coords beforeLinearConverterModeluses them. Usingself.flows[flow].label_fullin the fit-name keeps labels unambiguous, and mutatingself.conversion_factorsto hold DataArrays after transformation is safe given how it’s used later.Also applies to: 224-237
437-475: Storage.transform_data consistently fits all temporal/periodic parametersThe storage refactor routes all charge-state bounds, efficiencies, losses, initial/final states, and capacity into
_fit_coords, with correct handling of the'lastValueOfSim'sentinel and the InvestParameters vs scalar capacity cases. This matches howStorageModellater constructs_relative_charge_state_boundsand investment constraints.
719-724: Transmission.transform_data correctly prepares loss parametersTransforming
relative_lossesandabsolute_lossesvia_fit_coordsafter the base Component transformation ensures loss series are aligned to the FlowSystem grid beforeTransmissionModelinspects them (e.g., to decide whether to add on/off binaries) and builds constraints.flixopt/interface.py (7)
81-84: LGTM!The refactor cleanly delegates coordinate fitting to the internal helper
_fit_coords, removing the explicitFlowSystemparameter dependency.
227-229: LGTM!The nested
transform_datacalls correctly propagate only thename_prefix, aligning with the new internal coordination model.
453-455: LGTM!Consistent with the refactoring pattern across all piecewise classes.
665-668: LGTM!The refactor maintains consistency across all piecewise effect classes.
1295-1320: LGTM!The refactor successfully removes the
FlowSystemparameter and delegates to internal helpers. The transformation is complete and consistent.
81-81: All callsites oftransform_data()have been properly updated.Verification confirms that no callsite is passing the removed
flow_systemargument. All 15 identified calls throughout the codebase—inflow_system.py,elements.py,components.py, andinterface.py—have been updated to use the new signature with either zero arguments or the single optionalname_prefixparameter.
967-982: The review comment is incorrect.Lines 967-982 access
self.flow_systemthrough a property getter that contains defensive checks (structure.py:321-339). The property raises a RuntimeError if_flow_systemis not set, with a message to ensure the parent element is registered viaflow_system.add_elements()first. Additionally, _set_flow_system recursively sets the flow system reference on nested Interface objects when the parent element is registered. By the timetransform_data()is called during FlowSystem iteration through components, effects, and buses, the initialization is guaranteed. Any missing initialization will raise a descriptive error before reaching the code in question.Likely an incorrect or invalid review comment.
…implementing a two-phase modeling pattern: Phase 1 - Variable Creation (_create_variables()): - Called during __init__ - Creates all variables and submodels - No constraints allowed (to avoid circular dependencies) Phase 2 - Constraint Creation (_create_constraints()): - Called from FlowSystemModel.do_modeling() after all models exist - Safely accesses variables from child/sibling models - Creates all constraints and relationships
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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/components.py (1)
511-526: Bug: initial capacity bounds computed with swapped min/max.minimum_initial_capacity should use minimum_capacity; maximum_initial_capacity should use maximum_capacity. Current code reverses them, leading to incorrect plausibility errors.
Apply this fix:
- 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) + minimum_initial_capacity = minimum_capacity * self.relative_minimum_charge_state.isel(time=0) + maximum_initial_capacity = maximum_capacity * self.relative_maximum_charge_state.isel(time=0)
🧹 Nitpick comments (1)
flixopt/structure.py (1)
328-354: Consider None passthrough in _fit_coords/_fit_effect_coords.Many callers pass optional values (possibly None). If FlowSystem.fit_* doesn’t handle None, add a fast return to avoid errors:
def _fit_coords(self, name: str, data, dims=None): - return self.flow_system.fit_to_model_coords(name, data, dims=dims) + if data is None: + return None + return self.flow_system.fit_to_model_coords(name, data, dims=dims) def _fit_effect_coords(self, prefix: str, effect_values, suffix: str = None, dims=None): - return self.flow_system.fit_effects_to_model_coords(prefix, effect_values, suffix, dims=dims) + if effect_values is None: + return None + return self.flow_system.fit_effects_to_model_coords(prefix, effect_values, suffix, dims=dims)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
flixopt/components.py(10 hunks)flixopt/elements.py(11 hunks)flixopt/interface.py(7 hunks)flixopt/structure.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
flixopt/elements.py (1)
flixopt/structure.py (8)
Element(918-966)Interface(258-915)register_class_for_io(45-54)_set_flow_system(292-305)flow_system(308-326)transform_data(276-290)_fit_coords(328-339)_fit_effect_coords(341-353)
flixopt/interface.py (3)
flixopt/components.py (2)
_set_flow_system(185-189)_set_flow_system(443-447)flixopt/elements.py (3)
_set_flow_system(103-109)_set_flow_system(252-256)_set_flow_system(485-491)flixopt/structure.py (6)
_fit_coords(328-339)_set_flow_system(292-305)flow_system(308-326)values(1268-1273)values(1555-1556)_fit_effect_coords(341-353)
flixopt/structure.py (3)
flixopt/components.py (5)
transform_data(221-228)transform_data(449-486)transform_data(731-735)_set_flow_system(185-189)_set_flow_system(443-447)flixopt/elements.py (6)
transform_data(111-117)transform_data(258-262)transform_data(493-517)_set_flow_system(103-109)_set_flow_system(252-256)_set_flow_system(485-491)flixopt/interface.py (10)
transform_data(81-84)transform_data(233-235)transform_data(465-467)transform_data(684-687)transform_data(956-1013)transform_data(1320-1351)_set_flow_system(227-231)_set_flow_system(459-463)_set_flow_system(677-682)_set_flow_system(950-954)
flixopt/components.py (3)
flixopt/elements.py (7)
_set_flow_system(103-109)_set_flow_system(252-256)_set_flow_system(485-491)transform_data(111-117)transform_data(258-262)transform_data(493-517)label_full(559-560)flixopt/interface.py (18)
_set_flow_system(227-231)_set_flow_system(459-463)_set_flow_system(677-682)_set_flow_system(950-954)transform_data(81-84)transform_data(233-235)transform_data(465-467)transform_data(684-687)transform_data(956-1013)transform_data(1320-1351)has_time_dim(203-204)has_time_dim(207-210)has_time_dim(441-442)has_time_dim(445-448)has_time_dim(667-668)has_time_dim(671-675)items(450-457)InvestParameters(691-1107)flixopt/structure.py (10)
_set_flow_system(292-305)flow_system(308-326)transform_data(276-290)label_full(943-944)label_full(1435-1436)values(1268-1273)values(1555-1556)items(1275-1280)items(1549-1550)_fit_coords(328-339)
🔇 Additional comments (33)
flixopt/elements.py (8)
103-110: FlowSystem propagation to nested interfaces/flows — looks good.Propagation covers on_off_parameters and all connected flows; aligns with new linkage model.
111-118: Confirm intentional omission of name_prefix for Flow.transform_data.Flows are transformed without the parent prefix. Please confirm this is intentional and consistent with scenario-equality naming (variables end with |flow_rate/|size and rely on element.label_full only).
252-257: Bus: FlowSystem propagation — OK.Propagation to all connected flows is consistent with Component behavior.
258-263: Bus: penalty fitting via _fit_coords — OK.Correctly aligns penalty to model coords with prefix; no concerns.
485-492: Flow: propagate FlowSystem to nested params — OK.Covers on_off_parameters and InvestParameters size; matches Interface contract.
493-518: Flow: coordinate/effect fitting centralization — OK.Comprehensive use of _fit_coords/_fit_effect_coords and InvestParameters handling looks correct.
592-604: Total flow hours tracking: bounds and coords look correct.Lower/upper bounds derived from element values, coords set to period/scenario; no issues spotted.
823-837: Bus balance with excess variables — signs are correct.Adjusting lhs by -(−excess_in + excess_out) yields inputs + excess_in = outputs + excess_out as intended.
flixopt/structure.py (6)
103-109: Model creation order — OK.Effects first, then components, then buses: consistent with dependencies.
276-289: Interface.transform_data contract update — OK.Doc reflects prefix-only API and internal FlowSystem access; consistent across modules.
292-306: Internal FlowSystem linkage — OK.Centralized _set_flow_system is clear and allows explicit propagation downstream.
307-327: Interface.flow_system accessor — good defensive check.Helpful error if not linked; message guides correct usage.
932-933: Initialize Element._flow_system — OK.Prepares elements for late linkage; aligns with new access pattern.
1494-1499: Submodel modeling-order doc — OK.Guidance to create nested submodels before constraints is appropriate.
flixopt/interface.py (7)
81-85: Piece.transform_data via _fit_coords — OK.Dims toggle via has_time_dim is clear; no issues.
227-232: Piecewise: FlowSystem propagation to Pieces — OK.
233-236: Piecewise: prefixed per-piece transform — OK.
459-468: PiecewiseConversion: propagation + prefixed transform — OK.
950-955: InvestParameters: propagate FlowSystem to PiecewiseEffects — OK.
956-1014: InvestParameters.transform_data — linked_periods guard looks right.Using flow_system.periods with clear error/warnings; fitting effects and bounds via wrappers is consistent.
1320-1351: OnOffParameters.transform_data — OK.Effect fitting and scalar bounds aligned to model coords.
flixopt/components.py (12)
185-190: LinearConverter: FlowSystem propagation to PiecewiseConversion — OK.
221-229: LinearConverter.transform_data — OK.Parent prefixing + conversion factor transform + delegated piecewise conversion looks consistent.
230-243: Conversion factor fitting — OK.Per-flow, per-equation prefixes ensure unique names; plausible alignment.
443-448: Storage: propagate FlowSystem to InvestParameters — OK.
449-487: Storage.transform_data — OK.Comprehensive fitting of all temporal/periodic parameters; InvestParameters branch handled.
731-736: Transmission.transform_data — OK.Loss parameters fitted via helpers; consistent with the new API.
750-752: TransmissionModel: modeling-order doc — OK.
790-795: LinearConverterModel: modeling-order doc — OK.
840-851: StorageModel: variable creation — OK.charge_state uses extra timestep coords; bounds from helper; no issues.
855-861: StorageModel: netto_discharge constraint — OK.Constraint matches definition; uses connected flow submodels.
879-897: StorageModel: investment + scaled bounds — OK.Submodel addition and bounds scaling based on investment size are consistent.
902-907: StorageModel: balanced sizes — OK.Equality constraint ties investment sizes; conditionally applied.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
flixopt/calculation.py (1)
101-101: Consider consolidating modeling state tracking.The new
_modeledflag introduces a second mechanism for tracking modeling state alongside the existingmodeledproperty (lines 172-173). This creates potential inconsistency:
- Line 209 checks
self.modeled(property that testsself.model is not None)- Line 233 checks
self._modeled(flag set after completion)These could diverge if
do_modeling()fails after settingself.modelbut before settingself._modeled = True.Consider updating the
modeledproperty to use this flag instead:@property def modeled(self) -> bool: - return True if self.model is not None else False + return self._modeledThis ensures consistent state tracking throughout the class.
flixopt/features.py (3)
339-365: PieceModel dims typing could be tightenedThe new explicit
inside_piece,lambda0, andlambda1variables plus theinside_piece == lambda0 + lambda1constraint look correct. Given thatself.dimscan be a tuple like('period', 'scenario'), you might want to adjust the type hint fromFlowSystemDimensions | Noneto something likeCollection[FlowSystemDimensions] | Noneto reflect actual usage and avoid confusing type checkers.
523-626: ShareAllocationModel: constraint wiring is correct; error message could be clearerThe pattern of:
- initializing
totalwithtotal == 0,- optionally adding
total_per_timestep == 0for time‑resolved models,- and adjusting the LHS of these constraints in
add_share()implements
total == Σ(non_time_shares) + Σ_t total_per_timestep(t)andtotal_per_timestep == Σ(time_shares)correctly. The guard against using per‑hour bounds without a time dimension is also good.Minor nit: the message
'Both max_per_hour and min_per_hour cannot be used when has_time_dim is False'doesn’t quite match the condition (it rejects either being set when'time' not in dims). Consider updating the message to say “eithermax_per_hourormin_per_hour” or adjusting the condition if the intent really was “both”.
399-456: PiecewiseModel segment constraints are correct; add clarifying comment on zero_point gate semanticsThe segment validation, lambda representation, and piece wiring are sound. The zero-point constraint
sum(piece.inside_piece) <= rhsis also correct: it gates segment activation—whenrhs=zero_point(a freely created binary), the piecewise is enabled only ifzero_point=1, matching test expectations and usage (e.g., gating toon_off.onin components.py).The existing constraint implementation is functionally correct. However, to clarify design intent for future readers, consider adding a brief comment near line 452–456 explaining that the freely-created
zero_pointacts as a gate: "When zero_point is enabled (=1), at most one segment is active; when disabled (=0), all segments remain inactive, effectively disabling the piecewise approximation."flixopt/effects.py (1)
663-683: EffectCollectionModel._do_modeling sequencing is reasonable but objective depends on penalty submodel stateThe new flow—creating all
EffectModelsubmodels, initializing the scalar penaltyShareAllocationModel, wiring cross‑effect shares, then setting the objective onobjective_effect.submodel.totalpluspenalty.total.sum()—is coherent and leverages the new share allocation abstraction.Because penalty shares from buses/components are added after this method (via
add_share_to_penalty), the objective expression relies on thepenalty.totalvariable being fully constrained later, which linopy supports. Just be aware of this temporal coupling when extending penalty logic in the future.flixopt/flow_system.py (3)
603-640: add_elements adds useful guardrails on types and ownershipThe new checks for allowed types (
Component,Effect,Bus), prior assignment to anotherFlowSystem, and global label uniqueness significantly reduce the chance of building inconsistent systems. The early_connected_and_transformedreset also avoids stale transformed state after adding new elements.
804-826: _validate_system_integrity currently only checks bus references; consider extending laterThe new integrity check that ensures every
Flow.buslabel exists inself.busesis sensible, even though_connect_networkalready raises if a bus is missing. It provides a central place to grow cross‑element validations (e.g., future checks on flows, components, or effects), so keeping the method and docstring generic is a good choice.
878-883: Manualflow_countcomputation avoids side‑effects when loggingComputing
flow_countfrom component inputs/outputs instead of callinglen(self.flows)is a nice touch to avoid triggering a cache rebuild during_connect_network. Logging remains informative without hidden work.flixopt/structure.py (1)
254-287: Interface.transform_data docs slightly out of sync with new signatureThe method signature now correctly takes only
name_prefix: str = '', and the note about accessing the FlowSystem viaself._flow_system/self.flow_systemis helpful. The class‑level docstring still mentionstransform_data(flow_system)as the required override, which could confuse users and implementers. Consider updating that bullet to reflect the new signature and usage.flixopt/elements.py (1)
587-613: FlowModel._do_modeling now clearly separates creation of flow variable, tracking, and sharesAdding a docstring and calling
super()._do_modeling()first, then:
- creating the
flow_ratevariable,- delegating to
_constraint_flow_rate()(which may add nested OnOff/Investment submodels),- adding the
total_flow_hourstracking viaModelingPrimitives.expression_tracking_variable,- then creating load factor bounds and effect shares,
matches the recommended “variables → submodels → constraints” pattern and keeps responsibilities well‑factored.
flixopt/components.py (1)
748-750: Consider more descriptive docstrings.The generic docstring "Create variables, constraints, and nested submodels" appears in three model classes but doesn't convey class-specific behavior. Consider adding specifics, e.g., for
TransmissionModel: "Create transmission efficiency equations and optional absolute loss constraints" or forStorageModel: "Create charge state variables, balance equations, and investment submodels."Also applies to: 789-791, 838-840
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
flixopt/calculation.py(3 hunks)flixopt/components.py(10 hunks)flixopt/effects.py(3 hunks)flixopt/elements.py(11 hunks)flixopt/features.py(8 hunks)flixopt/flow_system.py(5 hunks)flixopt/interface.py(7 hunks)flixopt/structure.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
flixopt/features.py (1)
flixopt/structure.py (1)
_do_modeling(1497-1504)
flixopt/calculation.py (2)
flixopt/structure.py (1)
do_modeling(98-107)flixopt/aggregation.py (1)
do_modeling(317-347)
flixopt/effects.py (2)
flixopt/structure.py (9)
label_full(947-948)label_full(1439-1440)_fit_coords(324-337)_fit_effect_coords(339-357)_do_modeling(1497-1504)values(1272-1277)values(1559-1560)create_model(943-944)add_submodels(70-78)flixopt/features.py (7)
_do_modeling(48-51)_do_modeling(174-245)_do_modeling(339-365)_do_modeling(399-456)_do_modeling(485-521)_do_modeling(556-582)ShareAllocationModel(524-627)
flixopt/components.py (2)
flixopt/structure.py (8)
_set_flow_system(288-301)flow_system(304-322)transform_data(272-286)values(1272-1277)values(1559-1560)items(1279-1284)items(1553-1554)_fit_coords(324-337)flixopt/interface.py (18)
_set_flow_system(223-227)_set_flow_system(455-459)_set_flow_system(673-678)_set_flow_system(942-946)transform_data(77-80)transform_data(229-231)transform_data(461-463)transform_data(680-683)transform_data(948-1005)transform_data(1308-1339)has_time_dim(199-200)has_time_dim(203-206)has_time_dim(437-438)has_time_dim(441-444)has_time_dim(663-664)has_time_dim(667-671)items(446-453)InvestParameters(687-1099)
flixopt/structure.py (5)
flixopt/core.py (1)
TimeSeriesData(33-141)flixopt/flow_system.py (3)
FlowSystem(38-1412)fit_to_model_coords(521-559)fit_effects_to_model_coords(561-584)flixopt/components.py (5)
transform_data(220-227)transform_data(448-485)transform_data(730-734)_set_flow_system(184-188)_set_flow_system(442-446)flixopt/elements.py (6)
transform_data(120-126)transform_data(267-271)transform_data(502-526)_set_flow_system(112-118)_set_flow_system(261-265)_set_flow_system(494-500)flixopt/interface.py (10)
transform_data(77-80)transform_data(229-231)transform_data(461-463)transform_data(680-683)transform_data(948-1005)transform_data(1308-1339)_set_flow_system(223-227)_set_flow_system(455-459)_set_flow_system(673-678)_set_flow_system(942-946)
flixopt/flow_system.py (3)
flixopt/structure.py (4)
label_full(947-948)label_full(1439-1440)Element(922-970)_set_flow_system(288-301)flixopt/components.py (2)
_set_flow_system(184-188)_set_flow_system(442-446)flixopt/elements.py (5)
Component(41-178)label_full(568-569)_set_flow_system(112-118)_set_flow_system(261-265)_set_flow_system(494-500)
flixopt/interface.py (3)
flixopt/structure.py (7)
transform_data(272-286)_fit_coords(324-337)_set_flow_system(288-301)flow_system(304-322)values(1272-1277)values(1559-1560)_fit_effect_coords(339-357)flixopt/components.py (5)
transform_data(220-227)transform_data(448-485)transform_data(730-734)_set_flow_system(184-188)_set_flow_system(442-446)flixopt/elements.py (6)
transform_data(120-126)transform_data(267-271)transform_data(502-526)_set_flow_system(112-118)_set_flow_system(261-265)_set_flow_system(494-500)
flixopt/elements.py (2)
flixopt/structure.py (14)
Element(922-970)ElementModel(1571-1592)FlowSystemModel(81-251)Interface(254-919)_set_flow_system(288-301)flow_system(304-322)transform_data(272-286)label_full(947-948)label_full(1439-1440)_fit_coords(324-337)_fit_effect_coords(339-357)_do_modeling(1497-1504)add_submodels(70-78)create_model(943-944)flixopt/flow_system.py (1)
create_model(641-654)
⏰ 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 (32)
flixopt/calculation.py (2)
232-235: Auto-modeling convenience feature looks good.The automatic call to
do_modeling()before solving improves the user experience by ensuring the model is ready. The logging message clearly communicates what's happening.Note: This check uses
self._modeledwhilefix_sizes()at line 209 usesself.modeled(the property). Consider standardizing on one approach as mentioned in the earlier comment.
199-199: Correct placement of flag setting.Setting
self._modeled = Trueat the end of bothFullCalculation.do_modeling()andAggregatedCalculation.do_modeling()ensures the flag accurately reflects successful completion of all modeling operations.Note:
SegmentedCalculationdoesn't set this flag, but that appears intentional since it uses sub-calculations (each with their own_modeledstate) rather than having its own model.Also applies to: 333-333
flixopt/features.py (2)
174-245: OnOffModel_do_modelingordering looks consistent with Submodel guidanceCalling
super()._do_modeling()first and then creating variables/constraints and nested patterns keeps this feature aligned with the new Submodel contract. The reuse ofModelingPrimitives/BoundingPatternsis coherent; no issues spotted here.
485-521: PiecewiseEffectsModel modeling flow is coherentInitializing
sharesbefore constructing the nestedPiecewiseModel, then wiring origin and share variables via their.nameintopiecewise_variables, ensures that piecewise constraints apply directly to the created variables. The final call toadd_share_to_effectsusingself.shareslooks correct. No functional issues spotted.flixopt/effects.py (2)
395-435: EffectModel._do_modeling follows the new “variables → submodels → constraints” patternCreating the periodic and temporal
ShareAllocationModelsubmodels first, followed by the total effect variable and the coupling constrainttotal == temporal.total + periodic.total, matches the new Submodel guidance and keeps the flow of dependencies clear. Bounds passed into the submodels (maximum_*,minimum_*,min_per_hour,max_per_hour) align with their intended semantics.
342-377: Effect.transform_data correctly leverages new FlowSystem wrappersSwitching to
self._fit_coords/self._fit_effect_coordsand dropping the explicitflow_systemparameter simplifies the API and keeps all coordinate alignment centralized inFlowSystem. The chosen dims for temporal/periodic/total bounds (['period', 'scenario']) and for share_from_* (['time', 'period', 'scenario']vs['period', 'scenario']) are consistent with how the modeling layer consumes these arrays in EffectModel._do_modeling.FlowSystem linkage is guaranteed before transform_data is called: elements receive their
_flow_systemreference via_set_flow_system()during add_elements, before connect_and_transform triggers the transform_data calls. The property raises RuntimeError if not set, preventing silent failures. The refactor itself is correct and safe.flixopt/flow_system.py (3)
586-602: connect_and_transform now cleanly orchestrates transform + integrity checkUsing
element.transform_data()(without passing the FlowSystem) together with the new_set_flow_systemlinkage, followed by_validate_system_integrity(), gives a clear two‑phase setup: local data alignment then cross‑element validation. This is a nice simplification as long as all elements are registered viaadd_elements()/_add_*()(which they are in the provided code), so thatflow_systemis set before transform.
786-803: Single-owner enforcement via_check_if_element_already_assignedlooks correctGuarding against adding an
Elementthat already belongs to anotherFlowSystemusing the_flow_systemback‑reference is a good safety check and matches the new_set_flow_systemcontract. The error message is explicit about how to reuse elements (via.copy()), which should help users.
827-847: _add_effects/_add_components/_add_buses: FlowSystem linkage and cache invalidation are well placedLinking each element back to the
FlowSystemvia._set_flow_system(self)before adding it to the respective container ensures that subsequenttransform_data()calls can reliably useself.flow_system. Invalidating the flows cache once per batch of components/buses is efficient and avoids unnecessary recomputation.flixopt/structure.py (4)
27-36: New imports support FlowSystem linkage and stats helpersBringing in
FlowSystemDimensions,TimeSeriesData, and type aliases for numeric/effect types is consistent with the new_fit_coordsand_fit_effect_coordshelpers and the stats export functionality. No issues here.
288-358: FlowSystem linkage and coordinate helpers in Interface are well designed
_set_flow_system, theflow_systemproperty, and the_fit_coords/_fit_effect_coordswrappers nicely encapsulate access toFlowSystem.fit_to_model_coordsandfit_effects_to_model_coords. The explicitRuntimeErrorwhen an Interface isn’t yet linked to aFlowSystemprovides a clear failure mode if someone callstransform_data()too early.
927-937: Element now tracking_flow_systemenables single-owner checksAdding
self._flow_system: FlowSystem | None = NoneinElement.__init__is the foundation for_check_if_element_already_assigned()inFlowSystem, and aligns with the new Interface linkage API. This will not affect serialization, as_flow_systemis not part of the constructor parameters.
1497-1504: Submodel._do_modeling docstring clarifies expected modeling orderThe updated docstring spelling out “create nested submodels first, then constraints referencing their variables” matches how the refactored models are structured (e.g., in
FlowModel,ComponentModel,BusModel, and feature models). Keeping this guidance explicit should help keep future implementations consistent.flixopt/elements.py (6)
20-38: Updated imports support Interface-based linkage and typingImporting
InterfaceandFlowSystemModelfrom.structureplus theFlowSystemtype for TYPE_CHECKING is consistent with the new_set_flow_systempatterns and theisinstance(self.size, Interface)check inFlow. No issues.
112-127: Component._set_flow_system / transform_data integrate cleanly with new Interface APIPropagating
flow_systemfromComponentto itsOnOffParametersand all attachedFlowinstances ensures that all nested objects can safely use_fit_coordsand_fit_effect_coords. Thetransform_datamethod building a prefix fromname_prefixandlabel_full, then delegating toon_off_parametersand eachflow, matches the new per‑element transform contract used fromFlowSystem.connect_and_transform().
261-271: Bus._set_flow_system / transform_data correctly handle penalty dataForwarding the FlowSystem to connected flows in
_set_flow_systemand using_fit_coordsto alignexcess_penalty_per_flow_hourwith model coordinates (using the bus‑level prefix) is consistent with howBusModellater multiplies byhours_per_step. The dims and naming look coherent.
494-527: Flow._set_flow_system / transform_data correctly propagate linkage and fit all parameters
_set_flow_systemforwarding toon_off_parametersand tosizewhen it’s anInterface(i.e.,InvestParameters) ensures all nested parameter objects can access the FlowSystem.transform_datafits relative bounds, fixed profiles, per‑flow‑hour effects, total flow‑hour bounds, load factors, and size itself (when notInvestParameters) via the new helpers with a consistent prefix based onlabel_full.This keeps the Flow-level API aligned with the rest of the refactor without changing modeling semantics.
822-846: BusModel._do_modeling correctly integrates excess variables and penaltiesThe updated
_do_modeling:
- registers all connected flow_rate variables,
- enforces the bus balance constraint,
- conditionally adds non‑negative
excess_input/excess_outputvariables and adjusts the balance equation,- and pushes penalty terms into the global penalty ShareAllocationModel via
add_share_to_penalty,is consistent with the new effects/penalty structure. The use of
hours_per_step * excess_penalty_per_flow_hourto convert to per‑flow‑hour penalties is dimensionally correct.
869-923: ComponentModel._do_modeling sequencing is consistent with new Submodel guidelinesThe method now:
- ensures all relevant flows have
OnOffParameterswhere needed,- creates all
FlowModelsubmodels,- then (optionally) creates a component‑level on variable and associated
OnOffModel,- and finally enforces mutual exclusivity for
prevent_simultaneous_flows.This ordering respects the instruction to create nested submodels before adding constraints that depend on them and keeps the logic straightforward.
flixopt/components.py (6)
184-188: LGTM! Proper flow_system propagation.The conditional propagation to
piecewise_conversioncorrectly prevents errors when it'sNone, and the pattern aligns with the codebase's propagation strategy.
220-227: Consistent signature refactoring.The removal of the
FlowSystemparameter and delegation to internal coordinate-fitting utilities is correctly implemented. All nestedtransform_datacalls properly pass only the prefix.
229-241: Correctly updated to use internal fitting utility.The switch from receiving
FlowSystemas a parameter to usingself._fit_coordsis consistent with the refactoring pattern and maintains the same functionality.
442-446: Proper conditional propagation for InvestParameters.The
isinstancecheck correctly identifies whencapacity_in_flow_hoursis anInvestParametersinstance that needs flow_system propagation, while scalar values don't require it.
448-485: Well-structured transform_data implementation.The parallel structure between
_set_flow_system(lines 442-446) andtransform_dataconsistently handlesInvestParametersvs. scalar values forcapacity_in_flow_hours. All coordinate fitting properly uses the internal_fit_coordswrapper.
730-734: Consistent coordinate fitting for transmission losses.The signature update and use of
self._fit_coordsalign with the refactoring pattern. Note thatTransmissioncorrectly omits a_set_flow_systemoverride since it has no nestedInterfaceobjects requiring special propagation (the parentComponenthandleson_off_parameters).flixopt/interface.py (7)
77-80: Clean signature update.The removal of the
FlowSystemparameter and use ofself._fit_coordsmaintains the existing dimension logic while aligning with the refactoring pattern.
223-231: Correct propagation and transformation for nested Pieces.The
_set_flow_systemmethod properly iterates through allPieceobjects to propagate the reference, andtransform_dataconsistently enumerates pieces to create unique prefixes.
455-463: Appropriate iteration patterns for propagation and transformation.Using
.values()in_set_flow_system(no names needed) and.items()intransform_data(names needed for prefixes) is the correct approach for this dictionary-based structure.
673-683: Proper handling of dual piecewise structure.The implementation correctly manages both the singular
piecewise_originand the dictionary ofpiecewise_shares, ensuring all nestedPiecewiseobjects receive the flow_system reference and have distinct prefixes.
942-946: Proper propagation to nested PiecewiseEffects.The conditional check prevents errors when
piecewise_effects_of_investmentisNone, following the established pattern.
948-1005: Complex but well-structured transform_data implementation.The method correctly handles multiple effect types, piecewise effects, and the intricate linked_periods conversion logic. The multiple accesses to
self.flow_system.periods(lines 984, 991, 993, 995, 997, 999) assume_set_flow_systemhas been called first—this is the intended design, enforced by theflow_systemproperty'sRuntimeError.The warnings at lines 992-997 are appropriately informational rather than blocking, allowing
compute_linked_periodsto proceed with boundary-inclusive logic even whenstart/endaren't exact period values.
1308-1339: Correct coordinate fitting with appropriate dimensions.The distinction between time-varying parameters (consecutive constraints using default dims) and period-level aggregates (total constraints using
['period', 'scenario']) correctly reflects the semantic differences in these constraints.
Updated the modeled property to use the _modeled flag instead of checking self.model is not None, ensuring consistent state tracking throughout
the Calculation class.
2. features.py - PieceModel Type Hints
Improved the dims parameter type from FlowSystemDimensions | None to Collection[FlowSystemDimensions] | None in both PieceModel and
PiecewiseModel to accurately reflect actual usage with tuples like ('period', 'scenario').
3. features.py - ShareAllocationModel Error Message
Updated the validation error message from "Both max_per_hour and min_per_hour cannot be used..." to "max_per_hour and min_per_hour require
'time' dimension in dims" to match the actual condition being checked.
4. features.py - PiecewiseModel Zero-Point Documentation
Added clarifying comments explaining that the zero_point binary variable acts as a gate: when enabled (=1), at most one segment is active; when
disabled (=0), all segments remain inactive.
5. effects.py - Penalty Temporal Coupling
Added a comment in EffectCollectionModel._do_modeling noting that penalty shares are added after the objective is set, explaining the temporal
coupling in the design.
6. structure.py - Interface Documentation
Updated the class-level docstring to reflect the correct transform_data(name_prefix='') signature instead of the outdated
transform_data(flow_system).
7. components.py - Model Class Docstrings
Replaced generic docstrings with specific descriptions:
- TransmissionModel: "Create transmission efficiency equations and optional absolute loss constraints for both flow directions"
- LinearConverterModel: "Create linear conversion equations or piecewise conversion constraints between input and output flows"
- StorageModel: "Create charge state variables, energy balance equations, and optional investment submodels"
All changes improve code clarity, consistency, and maintainability without altering functionality.
… feature/simplify-structure # Conflicts: # flixopt/features.py
… feature/simplify-structure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
54-84: Accurate and well-structured changelog entries for architectural improvements.The Unreleased section clearly documents the internal restructuring work. However, a few items could be refined:
Lines 59, 62–63 (Added section): The "Helper methods in Interface base class" and "FlowSystem property in Interface" are internal/architectural plumbing rather than user-facing features. Consider moving these to the "Development" section or the "Changed" section if they're part of the internal refactoring story rather than new capabilities.
Lines 71 and 79 (duplication): Both the "Changed" section (line 71) and "Development" section (line 79) describe the two-phase modeling pattern in
_do_modeling(). This creates redundancy—consider consolidating or clarifying the distinction between what changed (line 71) and what was refactored (line 79).Apply this diff to improve clarity and reduce duplication:
### ✨ Added - **Auto-modeling**: `Calculation.solve()` now automatically calls `do_modeling()` if not already done, making the explicit `do_modeling()` call optional for simpler workflows - **System validation**: Added `_validate_system_integrity()` to validate cross-element references (e.g., Flow.bus) immediately after transformation, providing clearer error messages - **Element registration validation**: Added checks to prevent elements from being assigned to multiple FlowSystems simultaneously ### ♻️ Changed - **Refactored FlowSystem-Element coupling**: - Introduced `_set_flow_system()` method in Interface base class to propagate FlowSystem reference to nested Interface objects - Each Interface subclass now explicitly propagates the reference to its nested interfaces (e.g., Component → OnOffParameters, Flow → InvestParameters) - Elements can now access FlowSystem via `self.flow_system` property instead of passing it through every method call - Added `_fit_coords()` and `_fit_effect_coords()` convenience wrappers for cleaner data transformation code - **Simplified transform_data() signature**: Removed `flow_system` parameter from `transform_data()` methods - FlowSystem reference is now accessed via `self.flow_system` property - **Two-phase modeling pattern within _do_modeling()**: Clarified the pattern where `_do_modeling()` creates nested submodels first (so their variables exist), then creates constraints that reference those variables - eliminates circular dependencies in Submodel architecture - **Improved cache invalidation**: Cache invalidation in `add_elements()` now happens once after all additions rather than per element - **Better logging**: Centralized element registration logging to show element type and full label ### 🐛 Fixed - Fixed inconsistent argument passing in `_fit_effect_coords()` - standardized all calls to use named arguments (`prefix=`, `effect_values=`, `suffix=`) instead of mix of positional and named arguments ### 👷 Development - **Eliminated circular dependencies**: Implemented two-phase modeling pattern within `_do_modeling()` where nested submodels are created first (creating their variables), then constraints are created that can safely reference those submodel variables - Added comprehensive docstrings to `_do_modeling()` methods explaining the pattern - Added missing type hints throughout the codebase - Improved code organization by making FlowSystem reference propagation explicit and traceable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(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.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Documentation