New solution storing interface #505
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds persistent solution storage to FlowSystem (xr.Dataset | None), serializes/deserializable solution data, exposes per-element solution views, introduces build_model/solve/optimize/transform accessors, and updates tests and docs to use the new FlowSystem-centered optimization API. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant FlowSystem
participant OptimizeAccessor as Optimize
participant Solver
participant Storage as NetCDF
User->>FlowSystem: build_model(normalize_weights?)
FlowSystem->>FlowSystem: prepare & model (populate element variable names)
User->>Optimize: __call__(solver, normalize_weights)
Optimize->>FlowSystem: build_model(normalize_weights)
Optimize->>Optimize: invoke FlowSystem.solve(solver)
Optimize->>Solver: run solver on model
Solver-->>Optimize: model.solution (xr.Dataset)
Optimize->>FlowSystem: FlowSystem.solution = model.solution
FlowSystem->>FlowSystem: set has_solution, rename time coord if needed
User->>FlowSystem: to_netcdf(path) / to_dataset()
FlowSystem->>Storage: serialize dataset (prefix solution|*, solution_time)
User->>FlowSystem: from_netcdf(path)
FlowSystem->>FlowSystem: load dataset, extract solution vars, rename solution_time→time
User->>FlowSystem: components['X'].solution (Element.solution)
FlowSystem->>FlowSystem: filter solution by element _variable_names
FlowSystem-->>User: element-scoped xr.Dataset
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_solution_persistence.py (1)
170-178: Assertion on line 177 is always true.The assertion
assert len(boiler._constraint_names) >= 0will always pass since list length is never negative. The comment suggests some elements might have no constraints, but this test doesn't verify constraints are actually populated when they should be.Consider strengthening the test:
- # Boiler should have some constraints - assert len(boiler._constraint_names) >= 0 # Some elements might have no constraints + # Verify constraint_names is a list (even if empty for some elements) + assert isinstance(boiler._constraint_names, list) + # For Boiler specifically, we expect some constraints + # (adjust if Boiler genuinely has no constraints in this config)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
flixopt/flow_system.py(4 hunks)flixopt/optimization.py(1 hunks)flixopt/structure.py(4 hunks)tests/test_solution_persistence.py(1 hunks)
🔇 Additional comments (10)
flixopt/optimization.py (1)
263-265: LGTM! Solution stored on FlowSystem after successful solve.The placement is correct—after the status check (which raises on infeasibility) and before
Results.from_optimization()is called. This ensures element-level solution access is available when results are constructed.flixopt/flow_system.py (3)
205-206: LGTM! Solution attribute initialization.Clean initialization of the solution storage attribute. Correctly set to
Nonebefore any optimization is run.
547-565: LGTM! Solution serialization with proper namespace separation.The approach is sound:
solution|prefix prevents name collisions with FlowSystem configuration variables- Renaming
time→solution_timepreserves the full solution (which may have extra timesteps for final states) without interfering with FlowSystem's time index- The
has_solutionflag enables clean conditional restoration
587-648: LGTM! Solution deserialization with proper validation.The restoration logic correctly:
- Separates solution variables by prefix
- Rebuilds the solution dataset with original variable names
- Reverses the
solution_time→timerename- Uses defensive check (
has_solution and solution_vars) to handle edge casestests/test_solution_persistence.py (1)
1-42: Good test structure with comprehensive coverage.The test organization is clean with logical groupings:
TestSolutionOnFlowSystem: FlowSystem.solution lifecycleTestSolutionOnElement: Element.solution propertyTestVariableNamesPopulation: Post-modeling stateTestSolutionPersistence: Serialization roundtripsTestFlowSystemFileIO: NetCDF persistenceTestNoSolutionPersistence/TestEdgeCases: Edge casesThe parametrized fixture with unique IDs for parallel execution is a good pattern.
flixopt/structure.py (5)
111-119: LGTM! Variable/constraint names population after modeling.The implementation correctly:
- Iterates over all elements via
self.flow_system.values()- Checks for
submodel is not Nonebefore accessing- Extracts variable/constraint names from the submodel's variables and constraints
This enables per-element solution access after the model is built.
736-750: LGTM! Enhanced deserialization for non-constructor attributes.The approach cleanly separates constructor arguments from extra attributes (like
_variable_names,_constraint_names) by introspecting the class signature. This allows Element-specific serialization fields to be restored even when subclasses don't include them in their constructors.
988-1008: LGTM! Element constructor extended for serialization support.The optional
_variable_namesand_constraint_namesparameters enable clean deserialization while defaulting to empty lists for fresh elements. The leading underscore convention appropriately signals these are internal/serialization-related.
1022-1037: LGTM! Element.solution property with proper validation.The validation chain provides clear error messages for common issues:
- Element not linked to FlowSystem
- No solution available
- Element not modeled (no variable names)
The final indexing
self._flow_system.solution[self._variable_names]will raise aKeyErrorif variable names don't exist in the solution, which is appropriate—this would indicate a mismatch between the modeled element and the loaded solution.
1039-1054: LGTM! Element serialization extended for variable/constraint names.Clean implementation:
- Extends parent via
super()- Conditionally includes names only when non-empty (avoiding serialization bloat for un-modeled elements)
- Pairs correctly with the enhanced
_resolve_reference_structurefor round-trip fidelity
…me values:
# Old API (removed)
sankey.flows(timestep=100)
# New API
sankey.flows(select={'time': '2023-01-01 12:00'})
sankey.flows(select={'time': slice('2023-01-01', '2023-01-15')})
1. Carrier attribute on flow_rates/flow_hours
- Each flow variable now has a carrier attribute
- Enables filtering: flow_system.statistics.flow_rates['Boiler(Steam)'].attrs['carrier'] → 'steam'
2. Carrier filtering in sankey
- New filter option: select={'carrier': 'heat'}
- Example: fs.statistics.plot.sankey.flows(select={'carrier': 'steam'})
3. Carrier colors in sankey (subtle/transparent)
- Links: Colored by carrier with 40% opacity (e.g., rgba(255, 0, 0, 0.4))
- Bus nodes: Colored by their carrier color
4. Carrier colors in topology
- topology.plot(): Links colored by carrier (subtle)
- topology.infos(): Now includes carrier_color in edge data
- topology.plot_legacy(): Edges colored by carrier
5. Carrier-level balance plot
# Show all heat production/consumption system-wide
fs.statistics.plot.carrier_balance('heat')
# Energy units
fs.statistics.plot.carrier_balance('electricity', unit='flow_hours')
- Positive: Inputs to carrier buses (production/sources)
- Negative: Outputs from carrier buses (consumption/sinks)
- Colors from component.color for each flow
1. Centralized Hex Color Handling in color_processing.py 2. Simplified Carrier Retrieval in flow_system.py 3. Updated statistics_accessor.py 4. Updated topology_accessor.py
Uses matplotlib.colors.to_rgba() under the hood, which handles all standard color formats that ma
1. flow_system.py
- Added self._topology: TopologyAccessor | None = None in __init__
- Changed topology property to cache the accessor (like statistics)
- Added self._topology = None in _invalidate_model() for invalidation on structure changes
2. topology_accessor.py
- Added cached color properties in __init__:
- _carrier_colors
- _component_colors
- _bus_colors
- Added three new properties:
- carrier_colors - maps carrier name → hex color
- component_colors - maps component label → hex color
- bus_colors - maps bus label → hex color (from carrier)
- Updated infos() and plot() to use cached colors
3. statistics_accessor.py
- Removed _carrier_colors from __init__
- Changed carrier_colors to delegate to self._fs.topology.carrier_colors
- Added component_colors and bus_colors properties (delegating to topology)
- Updated _get_color_map_for_balance() to use cached component_colors
- Updated carrier_balance() to use cached component_colors
Architecture
FlowSystem
└── topology (cached TopologyAccessor)
├── carrier_colors ─┐
├── component_colors ├── Cached once, invalidated on structure change
└── bus_colors ─┘
└── statistics (cached StatisticsAccessor)
├── carrier_colors ─┐
├── component_colors ├── Delegates to topology
└── bus_colors ─┘
All 79 tests pass.
…created NaN values that caused a coordinate mismatch error.
Better Solution: Instead of silently filling gaps, we now:
1. Raise a clear error by default when resampling creates gaps, listing the affected variables
2. Require explicit fill_gaps parameter if the user wants to proceed:
- 'ffill' - forward fill (use previous value)
- 'bfill' - backward fill (use next value)
- 'interpolate' - linear interpolation
…ransformed (like after copy()), the add_elements method only invalidated if model is not None. After copy(), the model is None but _connected_and_transformed is True, so new elements' transform_data() was never called.
…to specify it (#517) * Change default size to None. Raise if not set and Status is used * Make maximum size mandatory (or fixed size). Update tests accordingly * Adding maximum sizes for FLows which get Status variables (also if only through component status * Update more tests * BUGFIX: Minimum or foixed size in storages * Added validation: 5. Component with status_parameters now validates that all flows have explicit sizes * In flixopt/elements.py - Flow._plausibility_checks(): - relative_minimum > 0 requires size (lines 585-590) - relative_maximum < 1 requires size (lines 592-596) - load_factor_min requires size (lines 598-603) - load_factor_max requires size (lines 605-609) * Added more validations * Fix test * Fix tests to specify size if needed * Improve check verbosity * Fix type hint * Improve conftest.py
* Change default size to None. Raise if not set and Status is used * Make maximum size mandatory (or fixed size). Update tests accordingly * Adding maximum sizes for FLows which get Status variables (also if only through component status * Update more tests * BUGFIX: Minimum or foixed size in storages * Added validation: 5. Component with status_parameters now validates that all flows have explicit sizes * In flixopt/elements.py - Flow._plausibility_checks(): - relative_minimum > 0 requires size (lines 585-590) - relative_maximum < 1 requires size (lines 592-596) - load_factor_min requires size (lines 598-603) - load_factor_max requires size (lines 605-609) * Added more validations * Fix test * Fix tests to specify size if needed * Improve check verbosity * Fix type hint * Improve conftest.py * Added more notebooks * Improve notebooks * Improve plotting of Piecewises * Improve notebooks * Streamline notebook * Remove old notebook * fix notebook * fix notebook script * Extract PlotResults class into separate module and reuse * Improve notebook * Add carrier units and effect units to accessors * Add carrier units and effect units to notebooks * Add carrier units and effect units to notebooks * Decrease notebook font size * Fix tests * Fix docs
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
Persistence
Deprecation
✏️ Tip: You can customize this high-level summary in your review settings.