Feature/minor improvements #556
Conversation
…key changes: Changes Made | Change | Lines Saved | Description | |------------------------------------------------|-------------|-------------------------------------------------| | Added _select_dims() helper | +8 lines | Reusable helper for period/scenario selection | | Simplified get_cluster_order_for_slice() | -6 lines | Reduced from 7 lines to 1 line | | Simplified get_cluster_occurrences_for_slice() | -5 lines | Reduced from 7 lines to 2 lines | | Simplified get_timestep_mapping_for_slice() | -6 lines | Reduced from 7 lines to 1 line | | Refactored expand_data() | -20 lines | Extracted _expand_slice helper, unified logic | | Simplified heatmap() | -40 lines | Replaced manual loops with vectorized np.repeat |
…ues for their previous_* parameters, causing TypeError: unsupported operand type(s) for -/+: '...' and 'NoneType'.
Fixes applied:
1. BoundingPatterns.state_transition_bounds (line 574-630):
- Updated type hint: previous_state: float | xr.DataArray | None = 0
- Made the initial constraint conditional: skip when previous_state is None (relaxed)
2. ModelingPrimitives.consecutive_duration_tracking (line 244-357):
- Updated type hint: previous_duration: xr.DataArray | None = 0
- Fixed big-M calculation to use 0 when previous_duration is None
- Made both initial and initial_lb constraints conditional
These None values come from features.py:218 and features.py:251 when there's no previous status/duration data, indicating a "relaxed" initial state with no constraint at t=0.
📝 WalkthroughWalkthroughRefactors clustering slice selection/expansion; makes modeling initial-state constraints optional; prefers solution time coordinates for temporal statistics templates; switches transforms to per-slice coords with outer-join concatenation; adds Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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[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 (4)
flixopt/modeling.py (2)
245-357: Clarifyprevious_durationtyping and documented constraint keysThe new
previous_durationsemantics look correct (skipping initial constraints whenNoneand using 0 inmega), but there are a couple of inconsistencies:
- The signature
previous_duration: xr.DataArray | None = 0conflicts with the default value and actual usage (you also accept plain scalars). This should probably bexr.DataArray | float | int | None(or similar) for clarity.- The docstring still states that
constraints_dictalways contains'initial'(and optionally'lb','initial_lb'), but now'initial'and'initial_lb'are omitted whenprevious_duration is None. That should be documented to avoid key‑access surprises.Consider tightening the type hint and updating the docstring to reflect that some keys are only present when
previous_durationis notNone.
580-635: Document optional initial constraint instate_transition_boundsAllowing
previous_state=Noneto relax the initial state (and returninginitial=None) is reasonable, but:
- The signature and return annotation now allow
initialto beNone, while the docstring still says “Tuple of (transition_constraint, initial_constraint, mutex_constraint)” without mentioning theNonecase.- Callers that previously assumed
initialwas always alinopy.Constraintneed to treat it as optional when they explicitly passprevious_state=None.I suggest explicitly noting in the docstring that
initialmay beNonewhenprevious_state is None, and double‑checking call sites to ensure they don’t unconditionally useinitialas a constraint.flixopt/clustering/base.py (2)
166-180: Guardget_cluster_occurrences_for_sliceagainst missing period/scenario selection
get_cluster_occurrences_for_slice()assumes that after_select_dimsthe result is 1‑D overcluster. Ifcluster_occurrencescarriesperiod/scenariodims and the caller forgets to passperiodand/orscenario,occ.sel(cluster=c).valueswill be multi‑dimensional andint(...)will raise.Consider either:
- Raising a clear
ValueErrorwhenoccstill has extra dims (period/scenario) and no selector was provided, or- Defining an aggregation behavior (e.g., sum or mean over remaining dims) when
period/scenarioare omitted.Right now the failure mode is a somewhat opaque
TypeErrorwhen converting non‑scalar arrays toint.
719-757: Heatmap per-timestep expansion assumes consistent time/cluster structureExpanding
cluster_ordervianp.repeat(..., timesteps_per_cluster, axis=0)and usingoriginal_timeas the time coordinate is a nice way to align the heatmap with actual timesteps.Two minor points to keep in mind:
- This implicitly assumes
len(original_time) == n_original_clusters * timesteps_per_cluster; if that ever drifts (e.g., due to partial final periods), DataArray construction will fail. A simple assertion on lengths here would make the failure mode clearer.- For the case where
original_datais absent, you fall back to an unlabeledtimeaxis, which is fine but worth being aware of for consumers expecting datetime coordinates.Functionally the change looks good and should fix the previous per‑cluster, non‑per‑timestep visualization.
🧹 Nitpick comments (1)
flixopt/clustering/base.py (1)
377-445:expand_dataassumes slices are 1‑D over time; future-proof the indexingThe
_expand_slice+ multi-slice recomposition logic correctly handles the clustered case (cluster,time) plusperiod/scenariodimensions. However, the function does not validate thatdata_slicecontains onlyclusterandtimedimensions after selecting by period/scenario.Looking at the aggregation code in
transform_accessor.py(lines 920–930), variables can be reshaped to('cluster', 'time') + other_dimswhereother_dimsare preserved from the original data. Although theclusters()function explicitly rejects such cases (line 825–829),expand_data()itself does not guard against them. If called with aggregated data containing extra dimensions (e.g.,('cluster', 'time', 'region')), the NumPy fancy indexing at line 408—data.values[cluster_ids, time_within]—would broadcast incorrectly and return an array with the wrong shape.To make the function robust against future changes or alternative clustering backends:
- Add an assertion before
_expand_slicethat verifiesdata_slice.dimsonly contains'cluster'(if present) and'time', or- Use explicit xarray indexing (e.g.,
.isel(cluster=..., time=...)) to preserve non-indexed dimensions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
flixopt/clustering/base.pyflixopt/modeling.pyflixopt/statistics_accessor.pyflixopt/transform_accessor.py
🧰 Additional context used
🧬 Code graph analysis (2)
flixopt/transform_accessor.py (3)
flixopt/interface.py (1)
items(454-461)flixopt/structure.py (5)
items(1536-1541)items(1810-1811)dims(229-231)values(1529-1534)values(1816-1817)flixopt/flow_system.py (2)
dims(1936-1956)coords(2001-2019)
flixopt/modeling.py (3)
flixopt/structure.py (2)
constraints(1710-1716)add_constraints(1617-1626)flixopt/results.py (2)
constraints(342-346)constraints(1271-1279)flixopt/flow_system.py (1)
isel(2371-2397)
🔇 Additional comments (4)
flixopt/statistics_accessor.py (1)
624-633: Temporal template now aligned with solution time coordinatesUsing
solution.coords['time']when available (and falling back toself._fs.timesteps) is a sensible improvement for expanded/modified solutions; no issues spotted.flixopt/clustering/base.py (1)
34-40: Good consolidation of period/scenario slicing via_select_dimsRefactoring
get_cluster_order_for_sliceandget_timestep_mapping_for_sliceto use_select_dimssimplifies the period/scenario handling without changing behavior. The helper is small and localized, which keeps things readable.Also applies to: 154-155, 365-376
flixopt/transform_accessor.py (2)
832-856: Clustering metrics: per-slicetime_seriescoords with NaN fallback look soundUsing each slice’s own DataFrame index as the
time_seriescoordinate (and falling back to a NaN‑filled array with a sample index when metrics are unavailable) is a reasonable way to handle differing metric coverage across (period, scenario) combinations.Combined with
_combine_slices_to_dataarray_generic’s outer join, this should:
- Preserve all time_series that appear in any slice, and
- Represent failed/empty metric slices cleanly as NaNs, rather than dropping them.
This looks like a solid fix for the earlier mismatch issues.
1147-1173: Outer-join in_combine_slices_to_dataarray_generic– safe for all current callersSwitching to
join='outer', fill_value=np.nanis appropriate for metrics wheretime_seriescoordinates may differ per slice. For other callers:
- cluster_weight, representative_weights: Use identical cluster dimensions across all slices (built from same
actual_n_clusters)- cluster_order, timestep_mapping, cluster_occurrences: All slices have identical base coordinates (timestep_mapping explicitly uses the same
original_timesteps_coordfor all slices; cluster_order and cluster_occurrences built from the same source dictionaries)Since base coordinates are identical across slices for these arrays, the outer join behaves identically to inner join—no NaNs introduced, dtypes preserved. The change is logically safe and improves robustness for the metrics path.
1. Fixed consecutive_duration_tracking type hint: previous_duration: xr.DataArray | float | int | None = None 2. Updated docstring to clarify which constraint keys are present based on parameters 3. Updated state_transition_bounds docstring to note initial_constraint is None when previous_state is None clustering/base.py: 1. Added guard in get_cluster_occurrences_for_slice - raises ValueError if period/scenario dims exist but weren't selected 2. Added length assertion in heatmap expansion - validates len(original_time) == expanded_values.shape[0] 3. Added dimension validation in expand_data._expand_slice - validates data slice only has expected dimensions before fancy indexing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
flixopt/clustering/base.py (2)
440-461: Consider documenting the iterative concatenation algorithm.The multi-dimensional concatenation logic (lines 452-461) correctly reconstructs the full DataArray by iteratively joining slices along each extra dimension in reverse order. However, the algorithm's nested loop structure and dict manipulation could benefit from a brief inline comment explaining the approach.
📝 Suggested documentation addition
# Concatenate iteratively along each extra dimension +# Algorithm: Build nested structure, then concatenate from innermost to outermost dimension +# Example: for dims [period, scenario], first concat all scenarios for each period, +# then concat all periods together result_arrays = expanded_slices for dim in reversed(extra_dims):
444-446: Consider extensibility: hardcoded period/scenario dimensions.Lines 444 and 446 unpack
**selectorto call_select_dims(...), which only acceptsperiodandscenarioparameters. This works currently becauseextra_dims(line 440) can only contain 'period' or 'scenario' based on the documented data model (line 268). However, if the data model is extended to support additional dimensions (e.g., 'region'), this code would fail with a TypeError.Consider either:
- Adding a comment documenting this assumption, or
- Making
_select_dimsmore generic to accept arbitrary dimension selections via **kwargs
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
flixopt/clustering/base.pyflixopt/comparison.pyflixopt/config.pyflixopt/modeling.pytests/deprecated/conftest.pytests/deprecated/test_component.pytests/deprecated/test_flow.pytests/deprecated/test_functional.pytests/test_component.pytests/test_flow.pytests/test_functional.py
🧰 Additional context used
🧬 Code graph analysis (5)
flixopt/comparison.py (1)
flixopt/structure.py (1)
to_dataset(914-937)
tests/deprecated/conftest.py (1)
flixopt/elements.py (1)
Flow(333-647)
flixopt/clustering/base.py (2)
flixopt/flow_system.py (3)
dims(1936-1956)sel(2299-2325)coords(2001-2019)flixopt/statistics_accessor.py (1)
_apply_selection(189-203)
tests/test_component.py (4)
tests/deprecated/conftest.py (1)
assert_conequal(756-772)tests/conftest.py (1)
assert_conequal(755-771)flixopt/structure.py (2)
constraints(1710-1716)variables(1719-1725)flixopt/transform_accessor.py (1)
isel(123-163)
flixopt/modeling.py (1)
flixopt/structure.py (3)
Submodel(1582-1761)constraints(1710-1716)add_constraints(1617-1626)
⏰ 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.14)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
🔇 Additional comments (26)
tests/deprecated/test_functional.py (1)
626-626: LGTM! API usage correctly updated.The addition of
previous_flow_rate=0properly enables initial constraint evaluation for uptime tracking. The inline comment clearly documents the purpose of this parameter.tests/deprecated/test_flow.py (3)
655-655: LGTM! Initial constraint parameter correctly added.The
previous_flow_rate=0parameter appropriately enables initial constraint testing for uptime tracking. The inline comment clearly explains its necessity.
819-819: LGTM! Well-documented initial state parameter.The
previous_flow_rate=0parameter enables downtime initial constraint evaluation. The detailed comment explaining "was OFF for 1h" provides helpful context for understanding the test setup.
987-987: LGTM! Startup constraint testing properly configured.The addition of
previous_flow_rate=0correctly enables initial constraint evaluation for startup/shutdown tracking. The comment clearly documents the requirement.tests/test_flow.py (3)
655-655: LGTM! Test correctly updated for initial constraint support.The
previous_flow_rate=0parameter appropriately enables testing of uptime initial constraints. The inline comment clearly explains the purpose.
819-819: LGTM! Initial downtime state well-documented.The addition of
previous_flow_rate=0correctly enables downtime initial constraint testing. The detailed comment about the previous "OFF for 1h" state provides valuable context.
987-987: LGTM! Startup tracking properly configured.The
previous_flow_rate=0parameter correctly enables initial constraint evaluation for startup tracking tests. Clear and helpful inline documentation.tests/test_functional.py (1)
607-607: LGTM! Initial constraint parameter correctly applied.The addition of
previous_flow_rate=0properly enables initial uptime constraint evaluation in this consecutive uptime/downtime test. The inline comment clearly documents the requirement.tests/deprecated/conftest.py (1)
619-621: LGTM! Fixture correctly updated with initial flow state.The refactoring to multi-line format improves readability, and the addition of
previous_flow_rate=0properly configures the BHKW2 fuel flow with an initial state. This aligns with the broader test suite updates for initial constraint support.tests/deprecated/test_component.py (1)
412-423: LGTM! Conditional constraint validation aligns with API changes.The conditional check correctly validates that the initial uptime constraint is only present when at least one flow has
previous_flow_rateset. This aligns with the modeling API change that makes initial constraints optional when no previous state is provided.tests/test_component.py (1)
412-423: LGTM! Test correctly validates conditional constraint behavior.The test properly verifies that the initial uptime constraint is only created when at least one flow provides
previous_flow_rate. The logic is consistent with the changes intests/deprecated/test_component.pyand correctly validates the new optional initial-state behavior.flixopt/comparison.py (1)
100-100: LGTM! Cache initialization follows established pattern.The
_inputscache initialization is consistent with the existing_solutionand_statisticscache attributes.flixopt/config.py (1)
167-176: LGTM! Dimension priority updated to support comparison features.The addition of
'case'and'variable'todim_priorityaligns with the newComparisonclass that uses a'case'dimension to combine multiple FlowSystems. The priority order is logical: time-related dimensions first, then case (for comparisons), then variable (for multi-variable plots), and finally analysis dimensions (cluster/period/scenario).flixopt/modeling.py (8)
254-254: LGTM! Parameter signature correctly allows None for optional initial constraints.The updated signature
previous_duration: xr.DataArray | float | int | None = Noneproperly expresses that initial constraints are optional. This provides flexibility for cases without prior state history.
281-292: LGTM! Docstring clearly documents the conditional constraint behavior.The updated docstring clearly explains:
- When
previous_durationisNone, no initial constraint is added (relaxed initial state)- The return dictionary conditionally includes 'initial', 'initial_lb' keys based on whether
previous_durationis providedThis provides clear guidance for API users.
296-297: LGTM! Big-M calculation correctly handles None.The big-M value correctly treats
Noneas0in the sum:(previous_duration if previous_duration is not None else 0). This ensures the constraint formulation remains valid when no initial duration is specified.
331-336: LGTM! Initial constraint correctly conditionalized.The initial constraint is now only created when
previous_durationis notNone, aligning with the documented relaxed initial state behavior. The constraint logic itself remains unchanged and correct.
348-358: LGTM! Edge case handling for minimum duration correctly guarded.The nested condition properly checks both:
previous_duration is not None(line 348) - ensures we have a previous durationprev > 0 and prev < min0(line 355) - validates the edge caseThe float conversion at line 350-352 is safe because it's inside the
if previous_duration is not Noneguard. This prevents forcing the initial state on when no previous history exists.
591-591: LGTM! Parameter signature maintains backward compatibility.The signature
previous_state: float | xr.DataArray | None = 0correctly:
- Allows
Noneto relax initial constraints (new capability)- Defaults to
0to maintain backward compatibility (existing behavior)This is a good design that doesn't break existing code.
611-617: LGTM! Docstring accurately documents optional initial constraint behavior.The docstring clearly explains:
previous_statecan beNoneto skip the initial constraint- Default is
0(maintaining backward compatibility)- Return tuple's
initial_constraintcan beNonewhenprevious_stateisNoneThis provides clear guidance for API users.
630-636: LGTM! Initial constraint correctly conditionalized.The initial constraint is only created when
previous_state is not None, with a clear else branch settinginitial = None. This implements the documented relaxed initial state behavior correctly.flixopt/clustering/base.py (5)
34-41: LGTM! Good abstraction to reduce duplication.The helper function cleanly encapsulates the conditional dimension selection logic, making the code more maintainable.
166-188: LGTM! Excellent defensive validation.The added validation ensures that period/scenario dimensions are explicitly selected when present, preventing subtle bugs from unintended broadcasting or aggregation. The error message clearly guides users to provide the necessary selectors.
416-431: LGTM! Excellent defensive validation.The
_expand_slicehelper includes robust validation (lines 418-426) that catches dimension mismatches early, preventing subtle indexing bugs. The error message clearly identifies the issue and guides users to apply proper selections.
154-164: LGTM! Cleaner code through helper delegation.Both methods now delegate to the
_select_dimshelper, reducing duplication and improving maintainability.Also applies to: 374-384
744-766: LGTM! Robust validation prevents silent data mismatches.The length consistency check (lines 749-756) catches potential bugs early when the expanded cluster data doesn't align with the original time coordinates. The error message provides clear diagnostic information including the shape mismatch and the parameters involved.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @CHANGELOG.md:
- Around line 295-299: Update CHANGELOG.md to document four missing user-facing
changes: under "✨ Added" add an entry noting the new public `inputs` property
exposed on the FlowSystem accessor in comparison.py; under "♻️ Changed" add
entries for config.py noting the default plotting `dim_priority` now includes
the 'case' dimension, for statistics_accessor.py noting the temporal template
now prefers solution-provided time coordinates when available, and for
transform_accessor.py noting per-slice `time_series` coordinate handling for
metric slices was updated to ensure proper dimension alignment; keep each entry
concise, reference the class/property names (`FlowSystem.inputs`,
`dim_priority`, "temporal template", "time_series" per-slice handling) and place
them in the appropriate sections for v6.0.0.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md
⏰ 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.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.14)
| ### 🐛 Fixed | ||
|
|
||
| - `temporal_weight` and `sum_temporal()` now use consistent implementation | ||
| - `FlowSystem.from_old_results()` now sets `previous_flow_rate=0` for flows of components with `status_parameters`, fixing startup cost calculation mismatch when re-optimizing migrated v4 results | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and check the files mentioned in the review
fd -t f 'comparison\.py|config\.py|statistics_accessor\.py|transform_accessor\.py' --type fRepository: flixOpt/flixopt
Length of output: 212
🏁 Script executed:
# Check for comparison.py with new inputs property
rg -l 'comparison\.py' | head -5Repository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
# Search for "inputs" property definition in comparison files
rg 'def inputs|@property.*inputs' --type py -A 3Repository: flixOpt/flixopt
Length of output: 323
🏁 Script executed:
# Check config.py for dim_priority changes
rg 'dim_priority' --type py -B 2 -A 2Repository: flixOpt/flixopt
Length of output: 4525
🏁 Script executed:
# Check statistics_accessor.py for temporal template changes
rg 'temporal|template' --type py -B 2 -A 2 | grep -A 10 -B 10 'statistics_accessor'Repository: flixOpt/flixopt
Length of output: 12913
🏁 Script executed:
# Check transform_accessor.py for coordinate handling
rg 'time_series|coordinates' --type py -B 2 -A 2 | grep -A 10 -B 10 'transform_accessor'Repository: flixOpt/flixopt
Length of output: 8690
🏁 Script executed:
# Read config.py to find the default dim_priority value
cat flixopt/config.py | head -100 | tail -50Repository: flixOpt/flixopt
Length of output: 2143
🏁 Script executed:
# Search for the _DEFAULTS dictionary to see the actual dim_priority default
rg "_DEFAULTS.*plotting" -A 20 flixopt/config.pyRepository: flixOpt/flixopt
Length of output: 2205
🏁 Script executed:
# Find the _DEFAULTS dictionary definition
rg "_DEFAULTS\s*=" flixopt/config.py -A 50 | head -100Repository: flixOpt/flixopt
Length of output: 1603
🏁 Script executed:
# Search more broadly for dim_priority tuple definition
rg "dim_priority.*=" flixopt/config.py -B 2 -A 2Repository: flixOpt/flixopt
Length of output: 636
CHANGELOG for v6.0.0 is incomplete — four user-facing code changes lack documentation.
The codebase contains these confirmed changes not documented in the CHANGELOG:
- comparison.py: New public
inputsproperty exposed on FlowSystem accessor for concatenated input data - config.py: Default plotting
dim_priorityexpanded to include 'case' dimension in priority order - statistics_accessor.py: Temporal template now uses solution-provided time coordinates when available to handle expanded solutions correctly
- transform_accessor.py: Per-slice time_series coordinate handling updated for metric slices to ensure proper dimension alignment in multi-dimensional cases
Add entries for these user-facing changes under the appropriate sections (✨ Added for new properties, ♻️ Changed for modifications).
🤖 Prompt for AI Agents
In @CHANGELOG.md around lines 295 - 299, Update CHANGELOG.md to document four
missing user-facing changes: under "✨ Added" add an entry noting the new public
`inputs` property exposed on the FlowSystem accessor in comparison.py; under "♻️
Changed" add entries for config.py noting the default plotting `dim_priority`
now includes the 'case' dimension, for statistics_accessor.py noting the
temporal template now prefers solution-provided time coordinates when available,
and for transform_accessor.py noting per-slice `time_series` coordinate handling
for metric slices was updated to ensure proper dimension alignment; keep each
entry concise, reference the class/property names (`FlowSystem.inputs`,
`dim_priority`, "temporal template", "time_series" per-slice handling) and place
them in the appropriate sections for v6.0.0.
Minor improvements and bug fixes
Summary
Changes
🐛 Bug Fixes
✨ Enhancements
📝 Documentation
Test plan