Invalidate and lock FLow system based on solution existance #518
Conversation
I've implemented the FlowSystem locking behavior as discussed. Here's what was done:
Core Concept
A FlowSystem is locked (read-only) when solution is not None.
New Features
1. is_locked property (flow_system.py:1127-1131)
- Returns True if the FlowSystem has a solution
2. reset() method (flow_system.py:1145-1168)
- Clears: solution, model, element submodels, variable/constraint names
- Returns self for method chaining
- Allows the FlowSystem to be modified and re-optimized
3. _invalidate_model() helper (flow_system.py:1133-1143)
- Called when adding elements/carriers to a FlowSystem with a model (but no solution)
- Clears model and element state
4. copy() method (flow_system.py:733-767)
- Creates a fresh FlowSystem copy without solution/model
- Supports copy.copy() and copy.deepcopy()
Behavior Changes
| Operation | Before Optimization | After build_model() | After optimize() |
|----------------|-------------------------------|------------------------------------|-------------------------------|
| add_elements() | ✓ Works | ⚠ Works, warns & invalidates model | ✗ RuntimeError |
| add_carriers() | ✓ Works | ⚠ Works, warns & invalidates model | ✗ RuntimeError |
| copy() | Returns copy without solution | Returns copy without solution | Returns copy without solution |
| reset() | No-op | Clears model | Clears solution & model |
Bug Fix
Fixed an issue where StatusParameters created during modeling (for prevent_simultaneous_flows and component-level status) weren't linked to the FlowSystem
(elements.py:957-964, components.py:731-732).
Tests
Added comprehensive tests in tests/test_flow_system_locking.py (28 tests) covering:
- is_locked property behavior
- add_elements() locking and invalidation
- add_carriers() locking and invalidation
- reset() method functionality
- copy() method functionality
- Loaded FlowSystem behavior
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughReplaces internal Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
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 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
🧹 Nitpick comments (4)
flixopt/flow_system.py (1)
733-776: FlowSystem.copy() correctly clones structure without optimization stateThe copy implementation strips solution/model/submodels by serializing with solution temporarily cleared and reconstructing via from_dataset, producing an unlocked, modifiable FlowSystem while leaving the original untouched and still locked if it had a solution.
You might consider using
type(self).from_dataset(...)instead ofFlowSystem.from_dataset(...)to make copy() friendlier to potential subclasses, but this is purely optional.flixopt/elements.py (3)
114-124: Component.link_to_flow_system/transform_data: propagation looks consistent, prefix param effectively unusedUsing
self.label_fullas the canonical prefix and pushing the FlowSystem intostatus_parametersand all flows matches the new prefix-based design, and the transform path correctly delegates to nested interfaces. Theprefixargument onComponent.link_to_flow_systemis intentionally ignored per docstring; if you don't plan to ever support caller-provided prefixes here, consider asserting thatprefixis empty or equal toself.label_fullto catch misuse early, or documenting it explicitly in the public API reference.Also applies to: 125-130
512-522: Flow.link_to_flow_system/transform_data: end‑to‑end prefix usage is coherent; minor type check nitThe new Flow linking and transform logic looks internally consistent:
label_fullis the stable prefix root,status_parametersandsize(when interface-based) get nested prefixes, and all fitted coordinates now key offself.prefix, which matches the rest of the system. One small polish point:link_to_flow_systemchecksisinstance(self.size, Interface)whiletransform_datalater checksisinstance(self.size, InvestParameters); given the constructor type hint (Numeric_PS | InvestParameters), you could tighten the first check toInvestParametersas well for clarity and to avoid surprising behavior if anotherInterfacesubtype is ever passed assize.Also applies to: 523-555
962-976: ComponentModel: newly created flow StatusParameters now correctly linked to FlowSystemLinking auto-created
StatusParameters()for flows (both fromstatus_parametersandprevent_simultaneous_flows) toself._model.flow_systemwith a flow-specific prefix fixes the previous missing-context issue and ensures theirtransform_datacan run safely. For consistency with other places that build prefixes, you might preferflow._sub_prefix('status_parameters')over manually formattingf'{flow.label_full}|status_parameters', but this is purely cosmetic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
flixopt/components.py(5 hunks)flixopt/effects.py(2 hunks)flixopt/elements.py(4 hunks)flixopt/flow_system.py(6 hunks)flixopt/interface.py(7 hunks)flixopt/structure.py(2 hunks)tests/test_flow_system_locking.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
flixopt/effects.py (4)
flixopt/components.py (5)
link_to_flow_system(183-187)link_to_flow_system(429-433)transform_data(219-225)transform_data(435-473)transform_data(717-720)flixopt/elements.py (7)
link_to_flow_system(114-123)link_to_flow_system(274-281)link_to_flow_system(512-521)label_full(596-597)transform_data(125-130)transform_data(283-286)transform_data(523-554)flixopt/interface.py (10)
link_to_flow_system(229-233)link_to_flow_system(461-465)link_to_flow_system(679-684)link_to_flow_system(907-911)transform_data(77-80)transform_data(235-237)transform_data(467-469)transform_data(686-689)transform_data(913-970)transform_data(1218-1241)flixopt/structure.py (8)
link_to_flow_system(346-383)flow_system(386-404)prefix(338-340)label_full(1080-1081)label_full(1626-1627)transform_data(322-335)_fit_coords(406-419)_fit_effect_coords(421-439)
flixopt/structure.py (3)
flixopt/components.py (5)
transform_data(219-225)transform_data(435-473)transform_data(717-720)link_to_flow_system(183-187)link_to_flow_system(429-433)flixopt/effects.py (2)
transform_data(247-290)link_to_flow_system(240-245)flixopt/interface.py (10)
transform_data(77-80)transform_data(235-237)transform_data(467-469)transform_data(686-689)transform_data(913-970)transform_data(1218-1241)link_to_flow_system(229-233)link_to_flow_system(461-465)link_to_flow_system(679-684)link_to_flow_system(907-911)
🔇 Additional comments (17)
flixopt/effects.py (2)
240-290: Effect prefix handling and transform_data look consistent with new Interface APIEffect.link_to_flow_system correctly uses label_full as the naming prefix and ignores the passed prefix as documented, and transform_data now uniformly relies on self.prefix for all coordinate and suffix construction, aligning with the new prefix-based naming scheme across the codebase. No issues spotted.
674-680: Penalty effect linkage via public API is correctUsing penalty_effect.link_to_flow_system(self._model.flow_system) ensures both user-defined and auto-created Penalty effects are properly linked and prefixed before modeling, replacing the old private setter cleanly.
tests/test_flow_system_locking.py (1)
1-282: Locking, reset, copy, and load tests comprehensively cover new semanticsThe test module exercises all key state transitions (lock/unlock, model invalidation, reset side effects, copy/deepcopy, and NetCDF round-trips) and directly matches the implemented error messages and behaviors in FlowSystem, giving strong confidence in the new API.
flixopt/structure.py (1)
318-384: Interface prefix/link_to_flow_system contract is well-defined and matches downstream usageThe new transform_data() contract, prefix/_sub_prefix helpers, and link_to_flow_system implementation provide a clear, centralized mechanism for FlowSystem linkage and naming that aligns with how Components, Elements, and Effects now propagate prefixes to nested interfaces. No issues detected.
flixopt/flow_system.py (3)
924-1014: Locking and model invalidation semantics in add_elements/add_carriers align with testsadd_elements/add_carriers now:
- Refuse mutations when is_locked is True, with clear RuntimeError messages mentioning reset().
- Emit a single warning and call _invalidate_model() when a model exists without a solution, resetting model and element submodels before adding new items.
This matches the new tests’ expectations and cleanly separates structural changes from optimization state.
1187-1230: is_locked and reset() provide a clear, minimal locking lifecycleis_locked simply reflects whether a solution is present, and reset() clears solution, model, connected flag, and element modeling artifacts while returning self. This is straightforward, predictable, and is well-covered by the new tests (including re-optimization after reset and behavior for loaded FlowSystems).
1455-1475: Using link_to_flow_system in _add_effects/_add_components/_add_buses is consistent with the new Interface contractReplacing the old private FlowSystem coupling with effect/component/bus.link_to_flow_system(self) ensures all elements (including those restored from datasets) receive both FlowSystem references and correct prefixes, harmonizing with the updated prefix-based transform_data logic.
flixopt/components.py (3)
183-226: LinearConverter flow-system linkage and transform_data are consistent and completeThe override of link_to_flow_system correctly delegates to the Component implementation and propagates the FlowSystem/prefix to piecewise_conversion via _sub_prefix, and transform_data now relies on super().transform_data plus local normalization of conversion_factors and piecewise_conversion without any name_prefix plumbing. Looks solid.
429-473: Storage linkage and prefix-based transform_data are well-aligned with the new APIStorage.link_to_flow_system properly forwards to the Component base and links InvestParameters-based capacity with a nested prefix, while transform_data systematically uses self.prefix for all storage parameters and respects the expected dims (time vs period/scenario) before plausibility checks. No issues found.
717-733: Transmission transform_data and automatic StatusParameters linkage for absolute losses are coherentTransmission.transform_data now uses self.prefix for relative/absolute loss naming after delegating to the base transform, and TransmissionModel.init ensures flows with absolute_losses have StatusParameters linked with a consistent status_parameters prefix. This integrates cleanly with the broader link_to_flow_system/prefix framework.
flixopt/elements.py (1)
274-282: Bus.link_to_flow_system/transform_data align with element-level semantics; check _fit_coords defaultsThe Bus override mirrors the Component pattern (always using
label_fullas prefix, then propagating to flows) andtransform_datanow consistently usesself.prefixfor coordinate naming, which should preserve naming stability as long aslink_to_flow_systemis called first. Please just confirm that_fit_coords(..., self.imbalance_penalty_per_flow_hour)without explicitdimsstill produces the same coordinate layout as before forNumeric_TPSinputs, to avoid subtle shape changes.Also applies to: 283-286
flixopt/interface.py (6)
77-81: Piece.transform_data: prefix‑based coords and has_time_dim handling look correctThe switch to
self.prefixandhas_time_dim‑drivendimsselection keeps the previous behavior while aligning with the new global transform contract; as long as everyPiecehas been linked vialink_to_flow_system(through its owningPiecewise), coordinate names and dimensions should remain consistent.
229-237: Piecewise.link_to_flow_system/transform_data: clean propagation to child piecesForwarding
prefixtoInterface.link_to_flow_systemand then assigning child prefixes asself._sub_prefix(f'Piece{i}')provides a stable naming hierarchy, and the no-argtransform_datathat simply iterates pieces matches that structure. This looks like a straightforward, correct port from the old name_prefix pattern.
461-469: PiecewiseConversion linkage and transform flow are consistent with nested Piecewise contractsThe new
link_to_flow_systemcorrectly sets the container’s prefix, then delegates to each nestedPiecewiseusing_sub_prefix(name), andtransform_datasimply walks those children. This matches howPiecewiseitself now behaves and should keep all origin/flow-specific piecewise data in sync under the same FlowSystem.
679-689: PiecewiseEffects: origin/effect prefixes and transform ordering are sensibleUsing
_sub_prefix('origin')forpiecewise_originand_sub_prefix(effect)for each share builds an intuitive tree (…|PiecewiseEffects|origin|Piece0,…|PiecewiseEffects|<effect>|Piece0, etc.), and the transform path (origin first, then all shares) keeps everything under the same FlowSystem context. This aligns well with how InvestParameters will now drive these structures.
907-912: InvestParameters.link_to_flow_system/transform_data: end‑to‑end integration with PiecewiseEffects and linked_periodsThe InvestParameters changes look coherent: you propagate the FlowSystem and prefix to any
piecewise_effects_of_investment, transform its tree withhas_time_dim=False, then normalize all investment‑related effects and sizing fields via_fit_effect_coords/_fit_coordsusingself.prefix. The linked‑periods tuple handling still guards against missing periods and then converts to a DataArray before fitting coordinates, which is good. Just ensure (in callers) thatlink_to_flow_systemis always invoked beforetransform_data, sinceself.flow_systemis now relied upon insidetransform_datafor the linked‑periods computation.Also applies to: 913-971
1218-1241: StatusParameters.transform_data: prefix usage and dimension choices remain consistentUsing
self.prefixfor both_fit_effect_coordscalls and all subsequent_fit_coordscalls (with explicit['period', 'scenario']where appropriate) matches the new system‑wide pattern and should preserve prior behavior for all status‑related parameters. The method is side‑effect‑only and doesn’t appear to introduce new edge cases beyond the existing data‑shape assumptions.
A public method for manual model invalidation when modifying element attributes after connect_and_transform():
def invalidate(self) -> FlowSystem:
"""Invalidate the model to allow re-transformation after modifying elements."""
- Raises RuntimeError if FlowSystem has a solution (must call reset() first)
- Returns self for method chaining
- Useful when you need to modify after connect_and_transform() but before optimize()
2. Updated docstrings
connect_and_transform() - Added comprehensive docstring explaining:
- What steps it performs
- Warning that attributes become xarray DataArrays after transformation
- Note about idempotency and how to reset with invalidate()
_invalidate_model() - Clarified its role and relationship to public methods
3. New tests (test_flow_system_locking.py:285-409)
Added TestInvalidate class with 8 tests:
- test_invalidate_resets_connected_and_transformed
- test_invalidate_clears_model
- test_invalidate_raises_when_locked
- test_invalidate_returns_self
- test_invalidate_allows_retransformation
- test_modify_element_and_invalidate - full workflow with reset
- test_invalidate_needed_after_transform_before_optimize - pre-optimization modification
- test_reset_already_invalidates - confirms reset already handles invalidation
Key insight from testing
reset() already calls _invalidate_model(), so modifications after reset() automatically take effect on the next optimize(). The new invalidate() method is primarily for the case where:
1. You've called connect_and_transform() manually
2. Haven't optimized yet (no solution)
3. Need to modify element attributes
* fix: Link status parameters to flow_system
* ⏺ Summary
I've implemented the FlowSystem locking behavior as discussed. Here's what was done:
Core Concept
A FlowSystem is locked (read-only) when solution is not None.
New Features
1. is_locked property (flow_system.py:1127-1131)
- Returns True if the FlowSystem has a solution
2. reset() method (flow_system.py:1145-1168)
- Clears: solution, model, element submodels, variable/constraint names
- Returns self for method chaining
- Allows the FlowSystem to be modified and re-optimized
3. _invalidate_model() helper (flow_system.py:1133-1143)
- Called when adding elements/carriers to a FlowSystem with a model (but no solution)
- Clears model and element state
4. copy() method (flow_system.py:733-767)
- Creates a fresh FlowSystem copy without solution/model
- Supports copy.copy() and copy.deepcopy()
Behavior Changes
| Operation | Before Optimization | After build_model() | After optimize() |
|----------------|-------------------------------|------------------------------------|-------------------------------|
| add_elements() | ✓ Works | ⚠ Works, warns & invalidates model | ✗ RuntimeError |
| add_carriers() | ✓ Works | ⚠ Works, warns & invalidates model | ✗ RuntimeError |
| copy() | Returns copy without solution | Returns copy without solution | Returns copy without solution |
| reset() | No-op | Clears model | Clears solution & model |
Bug Fix
Fixed an issue where StatusParameters created during modeling (for prevent_simultaneous_flows and component-level status) weren't linked to the FlowSystem
(elements.py:957-964, components.py:731-732).
Tests
Added comprehensive tests in tests/test_flow_system_locking.py (28 tests) covering:
- is_locked property behavior
- add_elements() locking and invalidation
- add_carriers() locking and invalidation
- reset() method functionality
- copy() method functionality
- Loaded FlowSystem behavior
* Add invalidation tests
* Add link_to_flow_system method
* Add link_to_flow_system method
* New invalidate() method (flow_system.py:1232-1275)
A public method for manual model invalidation when modifying element attributes after connect_and_transform():
def invalidate(self) -> FlowSystem:
"""Invalidate the model to allow re-transformation after modifying elements."""
- Raises RuntimeError if FlowSystem has a solution (must call reset() first)
- Returns self for method chaining
- Useful when you need to modify after connect_and_transform() but before optimize()
2. Updated docstrings
connect_and_transform() - Added comprehensive docstring explaining:
- What steps it performs
- Warning that attributes become xarray DataArrays after transformation
- Note about idempotency and how to reset with invalidate()
_invalidate_model() - Clarified its role and relationship to public methods
3. New tests (test_flow_system_locking.py:285-409)
Added TestInvalidate class with 8 tests:
- test_invalidate_resets_connected_and_transformed
- test_invalidate_clears_model
- test_invalidate_raises_when_locked
- test_invalidate_returns_self
- test_invalidate_allows_retransformation
- test_modify_element_and_invalidate - full workflow with reset
- test_invalidate_needed_after_transform_before_optimize - pre-optimization modification
- test_reset_already_invalidates - confirms reset already handles invalidation
Key insight from testing
reset() already calls _invalidate_model(), so modifications after reset() automatically take effect on the next optimize(). The new invalidate() method is primarily for the case where:
1. You've called connect_and_transform() manually
2. Haven't optimized yet (no solution)
3. Need to modify element attributes
* Typo
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.