Feature/398 feature facet plots in results (#419) #422
Conversation
* Add animation and faceting options to plots * Adjust size of the frame * Utilize plotly express directly * Rmeocve old class * Use plotly express and modify stackgroup afterwards * Add modifications also to animations * Mkae more compact * Remove height stuff * Remove line and make set opacity =0 for area * Integrate faceting and animating into existing with_plotly method * Improve results.py * Improve results.py * Move check if dims are found to plotting.py * Fix usage of indexer * Change selection string with indexer * Change behaviout of parameter "indexing" * Update CHANGELOG.md * Add new selection parameter to plotting methods * deprectae old indexer parameter * deprectae old indexer parameter * Add test * Add test * Add test * Add test * Fix not supportet check for matplotlib * Typo in CHANGELOG.md
WalkthroughAdds faceting and animation to plotting, broad xarray support and heatmap reshaping, introduces a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Results as CalculationResults
participant Plotting as flixopt.plotting
participant Xr as xarray
participant PX as PlotlyExpress
User->>Results: plot_heatmap(variable_name, select?, facet_by?, animate_by?)
Results->>Plotting: apply selection via _apply_selection_to_data(select)
Plotting->>Xr: detect xr.DataArray/Dataset?
alt xr input or reshape_time requested
Plotting->>Plotting: reshape_data_for_heatmap(reshape_time, facet_by, animate_by, fill)
Plotting->>Plotting: convert xr -> long-form DataFrame (if needed)
Plotting->>PX: px.imshow/px.line with facet/animation args
else DataFrame-only
Plotting->>PX: px.line/px.bar directly
end
PX-->>Plotting: go.Figure
alt engine == matplotlib
Plotting-->>User: (fig, ax)
else
Plotting-->>User: go.Figure
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
* Add animation and faceting options to plots * Adjust size of the frame * Utilize plotly express directly * Rmeocve old class * Use plotly express and modify stackgroup afterwards * Add modifications also to animations * Mkae more compact * Remove height stuff * Remove line and make set opacity =0 for area * Integrate faceting and animating into existing with_plotly method * Improve results.py * Improve results.py * Move check if dims are found to plotting.py * Fix usage of indexer * Change selection string with indexer * Change behaviout of parameter "indexing" * Update CHANGELOG.md * Add new selection parameter to plotting methods * deprectae old indexer parameter * deprectae old indexer parameter * Add test * Add test * Add test * Add test * Add heatmap support * Unify to a single heatmap method per engine * Change defaults * readd time reshaping * readd time reshaping * lengthen scenario example * Update * Improve heatmap plotting * Improve heatmap plotting * Moved reshaping to plotting.py * COmbinations are possible! * Improve 'auto'behavioour * Improve 'auto' behavioour * Improve 'auto' behavioour * Allow multiple variables in a heatmap * Update modeule level plot_heatmap() * remove code duplication * Allow Dataset instead of List of DataArrays * Allow Dataset instead of List of DataArrays * Update plot tests * FIx Missing renme in ElementResults.plot_heatmap() * Update API
* Add animation and faceting options to plots * Adjust size of the frame * Utilize plotly express directly * Rmeocve old class * Use plotly express and modify stackgroup afterwards * Add modifications also to animations * Mkae more compact * Remove height stuff * Remove line and make set opacity =0 for area * Integrate faceting and animating into existing with_plotly method * Improve results.py * Improve results.py * Move check if dims are found to plotting.py * Fix usage of indexer * Change selection string with indexer * Change behaviout of parameter "indexing" * Update CHANGELOG.md * Add new selection parameter to plotting methods * deprectae old indexer parameter * deprectae old indexer parameter * Add test * Add test * Add test * Add test * Add heatmap support * Unify to a single heatmap method per engine * Change defaults * readd time reshaping * readd time reshaping * lengthen scenario example * Update * Improve heatmap plotting * Improve heatmap plotting * Moved reshaping to plotting.py * COmbinations are possible! * Improve 'auto'behavioour * Improve 'auto' behavioour * Improve 'auto' behavioour * Allow multiple variables in a heatmap * Update modeule level plot_heatmap() * remove code duplication * Allow Dataset instead of List of DataArrays * Allow Dataset instead of List of DataArrays * Add tests * More examples * Update plot_charge state() * Try 1 * Try 2 * Add more examples * Add more examples * Add smooth line for charge state and use "area" as default * Update scenario_example.py * Update tests
|
@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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
88-91: Fix contradictory/typo’d notes around plotting parameter names.Bullets conflict: “continue to use
mode” vs “Renamed newmodeparameter tounit_type”. Also missing backtick.-### 🐛 Fixed -- Reverted breaking change from v3.0.0: continue to use `mode parameter in plotting instead of new `style` -- Renamed new `mode` parameter in plotting methods to `unit_type` +### 🐛 Fixed +- Reverted breaking change from v3.0.0: continue to use `mode` parameter in plotting instead of the new `style`. +### ♻️ Changed +- (Remove if not intended) If renaming is desired, clarify: “Renamed plotting parameter `style` back to `mode`.” Do not mention `unit_type` unless implemented.Please adjust according to the actual code (current public API uses
mode).
🧹 Nitpick comments (14)
tests/test_overlay_line_on_area.py (3)
105-111: Deep‑copy traces when moving them between figures/frames.Reusing the same trace objects from
charge_figinfig1/fig2/fig3/fig4/fig5(and frames) can cause shared‑UID/axis-binding issues and unexpected mutation. Clone the traces before adding.Apply this pattern once and reuse across all overlays:
+import copy as _copy ... - fig1.add_trace(trace) + fig1.add_trace(_copy.deepcopy(trace)) ... - fig3.frames[i].data = fig3.frames[i].data + (trace,) + fig3.frames[i].data = fig3.frames[i].data + (_copy.deepcopy(trace),)Also applies to: 155-161, 202-208, 214-221, 268-273, 275-284, 328-334
105-111: Tighten legend noise for overlay lines.Multiple identical “ChargeState” traces across subplots/frames can spam legends. Show one legend entry and hide the rest.
Example tweak:
for trace in charge_fig.data: trace.line.width = 3 trace.line.shape = 'linear' - trace.line.dash = 'dash' - fig1.add_trace(trace) + trace.line.dash = 'dash' + trace.showlegend = False + fig1.add_trace(_copy.deepcopy(trace)) +# Optionally turn on legend for the first added trace only: +# fig1.data[-1].showlegend = TrueAlso applies to: 155-161, 202-208, 268-273, 328-334
107-109: “linear” isn’t “smooth”.Comment says “Smooth line”, but
line.shape='linear'is straight segments. If you actually want smoothing, usespline; if you just want non‑stepped, keeplinearand update the comment.- trace.line.shape = 'linear' # Smooth line (not stepped like flows) + trace.line.shape = 'linear' # Straight (non-stepped) lineOr:
- trace.line.shape = 'linear' + trace.line.shape = 'spline'Also applies to: 158-160, 204-206, 217-219, 270-272, 281-283, 330-333
flixopt/plotting.py (4)
838-846: Simplify time‑step check; avoid pandas Series for deltas.Converting to
pd.Series(...).diff()is heavier and can introduce dtype quirks. Use NumPy timedeltas for a leaner, safer min‑step computation.- if data.sizes['time'] > 0: - time_diff = pd.Series(data.coords['time'].values).diff().dropna() - if len(time_diff) > 0: - min_time_diff_min = time_diff.min().total_seconds() / 60 + if data.sizes['time'] > 1: + tvals = pd.to_datetime(data.coords['time'].values) + deltas = (tvals[1:] - tvals[:-1]).astype('timedelta64[m]').astype(float) + if deltas.size > 0: + min_time_diff_min = np.nanmin(deltas)
628-641: Bar width from Timedelta may be odd in Matplotlib.
width = data.index.to_series().diff().dropna().min()yields a Timedelta; Matplotlib accepts numeric widths. Convert to days (float) for consistent rendering across backends.- width = data.index.to_series().diff().dropna().min() # Minimum time difference + dt = data.index.to_series().diff().dropna().min() + width = getattr(dt, "days", None) if dt is not pd.NaT else None + if width is None or width == 0: + # Fallback: one unit or 0.8*step in days + width = (dt / pd.Timedelta(days=1)) * 0.8 if pd.notna(dt) else 1.0
520-532: Group/stack modes: ensure consistent gap defaults.Minor nit: grouped bars use
bargap=0.2; stacked usesbargap=0. Consider exposingbargapas a kwarg or standardize defaults for visual consistency.No code change required; consider adding a
bar_gap: float | None = Nonekwarg and pass through toupdate_layout(...).
497-501: Color mapping determinism.
all_vars = df_long['variable'].unique().tolist()preserves first-seen order; with melted DataFrames this can vary. If you want stable colors across runs, sort the labels before assignment.- all_vars = df_long['variable'].unique().tolist() + all_vars = sorted(df_long['variable'].unique().tolist())tests/test_results_plots.py (1)
59-62: Facet/animate disabling for matplotlib is good; consider asserting no errors.Optional: add a quick smoke assertion that the call returns without raising and closes figures to keep CI quiet.
No diff necessary; consider wrapping
results.plot_heatmap(...)in a try/except andplt.close('all')in a finally.tests/test_plots.py (1)
64-66: Avoid writing artifacts to repo root; use a temp path.Saving PNGs into the working directory can clutter CI and developer machines. Prefer tempfile.TemporaryDirectory() or pytest’s tmp_path to write images, and remove them after assertions.
Also applies to: 73-75, 80-82, 88-90, 113-115, 141-143, 149-151, 157-159
examples/04_Scenarios/scenario_example.py (1)
85-91: Verify dimension alignment for vector parameters.
- relative_maximum_final_charge_state has length 3 (likely period).
- relative_loss_per_hour has length 2 (likely scenario).
- relative_maximum_charge_state has length 9*20 (likely time).
Make sure Storage maps these arrays to the intended dims (period/scenario/time). Passing raw numpy arrays without explicit xarray coords can silently misalign. Consider using xr.DataArray with named dims for clarity.
tests/test_select_features.py (1)
15-16: Avoid 'browser' renderer in tests.Setting pio.renderers.default = 'browser' can misbehave on headless CI. Use 'json' (safe, no GUI) or 'svg'/'png' with kaleido if needed.
flixopt/results.py (3)
2107-2161: Handle tuples/ndarrays/Indexes in coord filters.filter_dataarray_by_coord only treats list as multi-value; tuples, numpy arrays, and pandas Indexes are mishandled. Normalize all sequence-like inputs to a list to avoid false negatives.
Apply this diff:
@@ - def apply_filter(array, coord_name: str, coord_values: Any | list[Any]): + def apply_filter(array, coord_name: str, coord_values: Any | list[Any]): # Verify coord exists if coord_name not in array.coords: raise AttributeError(f"Missing required coordinate '{coord_name}'") - # Convert single value to list - val_list = [coord_values] if isinstance(coord_values, str) else coord_values + # Normalize to list for sequence-like inputs (excluding strings) + import numpy as _np + import pandas as _pd + if isinstance(coord_values, (list, tuple, _np.ndarray, _pd.Index)): + val_list = list(coord_values) + else: + val_list = [coord_values] @@ - return array.where( - array[coord_name].isin(val_list) if isinstance(coord_values, list) else array[coord_name] == coord_values, - drop=True, - ) + return array.where(array[coord_name].isin(val_list) if len(val_list) > 1 else array[coord_name] == val_list[0], + drop=True)
1335-1415: Overlay approach may desync facets/frames between figures.Adding traces/frames from a separately-built figure assumes identical subplot axis ids and frame ordering. If with_plotly evolves, axis ids can diverge. Prefer plotting both flows and charge_state from one combined dataset in a single with_plotly() call, or ensure axis references are remapped when merging figures.
197-203: Minor typo in deprecation warning."Acess is now by ..." → "Access is now via ...".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)examples/04_Scenarios/scenario_example.py(3 hunks)flixopt/plotting.py(4 hunks)flixopt/results.py(14 hunks)tests/ressources/Sim1--summary.yaml(1 hunks)tests/test_facet_plotting.py(1 hunks)tests/test_overlay_line_on_area.py(1 hunks)tests/test_plots.py(2 hunks)tests/test_results_plots.py(1 hunks)tests/test_select_features.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.13)
🔇 Additional comments (2)
tests/ressources/Sim1--summary.yaml (1)
1-92: LGTM — data fixture only.Structured test resource; no code changes required.
tests/test_plots.py (1)
107-113: Confirm 2D heatmap axis semantics.reshape_to_2d + dims=['timestep','timeframe'] assumes downstream heatmap helpers interpret those names consistently. If plotting.heatmap_with_* expects 'timeframe' x 'timestep' in a specific order, add an assertion or swap axes accordingly to avoid rotated plots.
| for trace in all_traces: | ||
| trace.stackgroup = variable_classification.get(trace.name, None) | ||
| # No opacity and no line for stacked areas | ||
| if trace.stackgroup is not None: | ||
| if hasattr(trace, 'line') and trace.line.color: | ||
| trace.fillcolor = trace.line.color # Will be solid by default | ||
| trace.line.width = 0 | ||
|
|
||
| # Update layout with basic styling (Plotly Express handles sizing automatically) |
There was a problem hiding this comment.
Area stack opacity likely too strong; align visuals with intent.
You set fillcolor = trace.line.color and line.width = 0, but no opacity. This makes areas fully opaque and can hide layers. If the goal is muted/stacked areas (and per PR notes “remove line for area traces”), set a sensible opacity.
for trace in all_traces:
trace.stackgroup = variable_classification.get(trace.name, None)
# No opacity and no line for stacked areas
if trace.stackgroup is not None:
if hasattr(trace, 'line') and trace.line.color:
- trace.fillcolor = trace.line.color # Will be solid by default
- trace.line.width = 0
+ trace.fillcolor = trace.line.color
+ trace.line.width = 0
+ # Make areas translucent so overlapping stacks remain readable
+ if getattr(trace, "opacity", None) is None:
+ trace.opacity = 0.6* Add animation and faceting options to plots * Adjust size of the frame * Utilize plotly express directly * Rmeocve old class * Use plotly express and modify stackgroup afterwards * Add modifications also to animations * Mkae more compact * Remove height stuff * Remove line and make set opacity =0 for area * Integrate faceting and animating into existing with_plotly method * Improve results.py * Improve results.py * Move check if dims are found to plotting.py * Fix usage of indexer * Change selection string with indexer * Change behaviout of parameter "indexing" * Update CHANGELOG.md * Add new selection parameter to plotting methods * deprectae old indexer parameter * deprectae old indexer parameter * Add test * Add test * Add test * Add test * Add heatmap support * Unify to a single heatmap method per engine * Change defaults * readd time reshaping * readd time reshaping * lengthen scenario example * Update * Improve heatmap plotting * Improve heatmap plotting * Moved reshaping to plotting.py * COmbinations are possible! * Improve 'auto'behavioour * Improve 'auto' behavioour * Improve 'auto' behavioour * Allow multiple variables in a heatmap * Update modeule level plot_heatmap() * remove code duplication * Allow Dataset instead of List of DataArrays * Allow Dataset instead of List of DataArrays * Add tests * More examples * Update plot_charge state() * Try 1 * Try 2 * Add more examples * Add more examples * Add smooth line for charge state and use "area" as default * Update scenario_example.py * Update tests * Handle extra dims in pie plots by selecting the first
- Replaced pandas Series diff() with NumPy np.diff() for better performance - Changed check from > 0 to > 1 (can't calculate diff with 0 or 1 element) - Converted to seconds first, then to minutes to avoid pandas timedelta conversion issues
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
flixopt/plotting.py (1)
552-563: Area traces: implement opacity and “mixed” styling as logged.Currently fully opaque fills hide layers; and “mixed ... dashed lines” isn’t implemented.
- for trace in all_traces: - trace.stackgroup = variable_classification.get(trace.name, None) - # No opacity and no line for stacked areas - if trace.stackgroup is not None: - if hasattr(trace, 'line') and trace.line.color: - trace.fillcolor = trace.line.color # Will be solid by default - trace.line.width = 0 + for trace in all_traces: + cls = variable_classification.get(trace.name, None) + trace.stackgroup = cls if cls in ('positive', 'negative') else None + if cls in ('positive', 'negative'): + if hasattr(trace, 'line') and trace.line.color: + trace.fillcolor = trace.line.color + trace.line.width = 0 + if getattr(trace, 'opacity', None) is None: + trace.opacity = 0.6 + elif cls == 'mixed': + # Show as dashed line, not stacked + if hasattr(trace, 'line'): + trace.line.width = 2 + trace.line.dash = 'dash' + if hasattr(trace, 'fill'): + trace.fill = Noneflixopt/results.py (1)
1789-1871: Fix TypeError: accept and forwardfillin plot_heatmap.SegmentedCalculationResults.plot_heatmap passes
fill, but module‑level plot_heatmap rejects it; this will crash.Apply:
def plot_heatmap( data: xr.DataArray | xr.Dataset, name: str | None = None, folder: pathlib.Path | None = None, colors: plotting.ColorType = 'viridis', save: bool | pathlib.Path = False, show: bool = True, engine: plotting.PlottingEngine = 'plotly', select: dict[str, Any] | None = None, facet_by: str | list[str] | None = None, animate_by: str | None = None, facet_cols: int = 3, reshape_time: tuple[Literal['YS', 'MS', 'W', 'D', 'h', '15min', 'min'], Literal['W', 'D', 'h', '15min', 'min']] | Literal['auto'] | None = 'auto', + fill: Literal['ffill', 'bfill'] | None = None, # Deprecated parameters (kept for backwards compatibility) indexer: dict[str, Any] | None = None, heatmap_timeframes: Literal['YS', 'MS', 'W', 'D', 'h', '15min', 'min'] | None = None, heatmap_timesteps_per_frame: Literal['W', 'D', 'h', '15min', 'min'] | None = None, color_map: str | None = None, ): @@ if engine == 'plotly': figure_like = plotting.heatmap_with_plotly( data=data, facet_by=facet_by, animate_by=animate_by, colors=colors, title=title, facet_cols=facet_cols, reshape_time=reshape_time, + fill=fill, ) default_filetype = '.html' elif engine == 'matplotlib': figure_like = plotting.heatmap_with_matplotlib( data=data, colors=colors, title=title, reshape_time=reshape_time, + fill=fill, ) default_filetype = '.png'Optional symmetry: add
fillto CalculationResults.plot_heatmap and forward it.#!/bin/bash rg -n "def plot_heatmap\(" flixopt/results.py -n -C2 rg -n "heatmap_with_(plotly|matplotlib)\(" flixopt/plotting.py -n -C2Also applies to: 1899-2101
🧹 Nitpick comments (2)
tests/test_heatmap_reshape.py (1)
19-30: Optional: make tests deterministic and add one assert per case.Seed RNG for reproducibility and assert a key size/value per test (e.g., 3×24 for daily/hourly).
+np.random.seed(42)You can also assert weekly/day labels count for ('W','D') to catch regressions.
Also applies to: 32-37, 39-51, 53-66, 68-76
flixopt/plotting.py (1)
719-889: reshape_to_2d appears unused; consider deprecating to reduce surface area.Mark as deprecated or remove if not referenced after the new reshape path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/CONTRIBUTING.md(1 hunks)flixopt/plotting.py(4 hunks)flixopt/results.py(16 hunks)tests/test_heatmap_reshape.py(1 hunks)tests/test_plots.py(0 hunks)tests/test_results_plots.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/test_plots.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_results_plots.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_heatmap_reshape.py (1)
flixopt/plotting.py (1)
reshape_data_for_heatmap(720-888)
flixopt/plotting.py (2)
flixopt/results.py (2)
size(1589-1597)sizes(433-460)flixopt/flow_system.py (3)
coords(769-775)resample(926-976)isel(889-924)
flixopt/results.py (2)
flixopt/flow_system.py (3)
coords(769-775)sel(852-887)to_dataset(243-255)flixopt/plotting.py (5)
with_plotly(329-577)with_matplotlib(580-673)export_figure(1794-1874)heatmap_with_plotly(1489-1680)heatmap_with_matplotlib(1683-1791)
⏰ 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.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
🔇 Additional comments (1)
.github/CONTRIBUTING.md (1)
15-15: No issues found—all extras are properly defined.The verification confirms that all three extras referenced in the pip install command ("full", "dev", "docs") are defined in
pyproject.tomlunder[project.optional-dependencies]. They are in sync and no action is needed.
…ith both positive and negative values) - Only stack "positive" and "negative" classifications, not "mixed" or "zero"
…914) - Added fill parameter to CalculationResults.plot_heatmap method (line 702) - Forwarded fill parameter to both heatmap_with_plotly and heatmap_with_matplotlib functions
- Added specific size assertions to all tests:
- Daily/hourly pattern: 3 days × 24 hours
- Weekly/daily pattern: 1 week × 7 days
- Irregular data: 25 hours × 60 minutes
- Multidimensional: 2 days × 24 hours with preserved scenario dimension
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
flixopt/results.py (2)
1785-1785: Consider aligning color defaults across methods.The
colorsdefault here is'portland', while other plot methods (plot_heatmap, plot_node_balance, plot_charge_state) and the module-level plot_heatmap all default to'viridis'. Unless'portland'is intentionally chosen for segmented heatmap visualizations, consider standardizing on'viridis'for consistency.
2342-2364: Optional: Consider renaming_apply_indexer_to_datafor consistency.The function name still references "indexer" but the parameter has been modernized to
select. Since this is an internal function (underscore prefix), it's not critical, but renaming to_apply_select_to_dataor_apply_selection_to_datawould align with the new naming convention.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
flixopt/results.py(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/results.py (2)
flixopt/flow_system.py (3)
coords(769-775)sel(852-887)to_dataset(243-255)flixopt/plotting.py (5)
with_plotly(329-587)with_matplotlib(590-683)export_figure(1760-1840)heatmap_with_plotly(1455-1646)heatmap_with_matplotlib(1649-1757)
⏰ 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 (6)
flixopt/results.py (6)
688-801: LGTM: Well-structured API modernization with comprehensive multi-variable support.The updated signature cleanly integrates faceting, animation, time reshaping, and multi-variable plotting. The delegation to the module-level function maintains separation of concerns, and passing deprecated parameters through for centralized handling is good design.
1040-1055: LGTM: Thorough deprecation handling and dimension safety checks.The conflict detection between deprecated
indexerand newselectprevents user confusion. The matplotlib dimension check (lines 1066-1072) correctly guards against multi-dimensional data that would break the single-axis matplotlib plotting path.Also applies to: 1066-1072
1187-1216: LGTM: Pragmatic auto-selection with clear user guidance.The auto-selection logic for extra dimensions is a sensible solution for pie charts (which require scalar data). The informative log messages (lines 1204-1207) clearly communicate what was auto-selected and guide users to override via
selectif needed. The handling of dimensions that may exist in only one dataset (lines 1197-1202) is robust.
1428-1472: LGTM: Clever figure composition for dual-mode visualization.The approach of creating separate figures for node balance and charge state, then combining their traces, elegantly handles the different visual representations (area/bar vs line). The frame combination logic (lines 1464-1471) is well-guarded, and the comment (line 1463) documents the assumption that both figures share the same animation structure.
2022-2042: LGTM: Robust multi-variable support and thorough dimension validation.The Dataset-to-DataArray conversion (lines 2022-2030) via
xr.concatalong a new'variable'dimension is the standard xarray pattern for multi-variable visualization. The matplotlib dimension checks (lines 2048-2066) comprehensively cover both time-reshaping scenarios and already-reshaped data, with clear error messages guiding users to solutions.Also applies to: 2048-2066
2307-2313: LGTM: Explicit type handling improves robustness.The updated normalization logic explicitly handles common sequence types (str, list, tuple, ndarray, Index) rather than relying on implicit duck-typing. This makes the function more predictable and easier to debug.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
flixopt/results.py (1)
1821-1856: Consider more robust conflict detection.The conflict detection relies on comparing against default values (lines 1824, 1844):
if reshape_time != 'auto': # Check if user explicitly set reshape_time if colors != 'portland': # Check if user explicitly set colorsThis approach could fail if:
- A user explicitly passes the default value (e.g.,
reshape_time='auto')- The default values change in the future
Consider using a sentinel value (e.g.,
_UNSET = object()) or tracking which parameters were explicitly provided through**kwargs.Example refactor:
_UNSET = object() def plot_heatmap( self, variable_name: str, reshape_time: ... = _UNSET, colors: ... = _UNSET, ... ): # Set actual defaults if not provided if reshape_time is _UNSET: reshape_time = 'auto' if colors is _UNSET: colors = 'portland' # Now check conflicts more reliably if (heatmap_timeframes is not None) and (reshape_time is not _UNSET): raise ValueError(...)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
flixopt/results.py(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/results.py (1)
flixopt/plotting.py (5)
with_plotly(329-587)with_matplotlib(590-683)export_figure(1760-1840)heatmap_with_plotly(1455-1646)heatmap_with_matplotlib(1649-1757)
⏰ 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.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
🔇 Additional comments (11)
flixopt/results.py (11)
20-23: LGTM: Runtime import optimization.Moving
plotlyunderTYPE_CHECKINGreduces runtime overhead since it's only needed for type annotations.
198-202: LGTM: Clear deprecation guidance.The updated warning clearly explains the distinction between
.flow_system_data(dataset) and.flow_system(restored FlowSystem object).
688-801: LGTM: Modern plotting API with backward compatibility.The method signature has been expanded with comprehensive faceting, animation, and time-reshaping capabilities while maintaining backward compatibility through deprecated parameters. All parameters are properly forwarded to the module-level
plot_heatmapfunction for centralized handling.
1040-1073: LGTM: Robust deprecation handling and dimension validation.The deprecation logic properly checks for conflicts between old (
indexer) and new (select) parameters. The Matplotlib dimension check (lines 1066-1072) correctly prevents MultiIndex issues by ensuring only thetimedimension remains after selection.
1187-1216: LGTM: Smart auto-selection for pie charts.The auto-selection logic elegantly handles the constraint that pie charts require scalar data. When extra dimensions remain after aggregation, the code automatically selects the first value while logging clear guidance for users who want different selections. This provides a good balance between convenience and explicitness.
1281-1296: LGTM: Consistent deprecation handling.The deprecation pattern matches other methods with proper conflict detection.
2022-2042: LGTM: Clean Dataset to DataArray conversion.The conversion of Dataset to DataArray with a new
variabledimension (lines 2028-2030) is straightforward and enables multi-variable heatmaps. The fallback title generation is sensible when no explicit name is provided.
2048-2066: LGTM: Thorough matplotlib constraint validation.The dimension validation correctly handles the constraint that matplotlib heatmaps require 2D data. The logic distinguishes between:
- Time dimension present + reshaping enabled → no extra dimensions allowed (lines 2053-2060)
- No time dimension or already reshaped → maximum 2 dimensions allowed (lines 2062-2066)
Error messages provide clear guidance on resolution options.
2307-2323: LGTM: Robust type handling for coordinate filters.The normalization logic correctly distinguishes between string values (which should not be iterated) and various sequence types. The optimization to use
==for single values vsisin()for multiple values is sensible.
1428-1472: Verify frame alignment when merging Plotly traces in animation scenarios.While both datasets are extracted from the same source with identical
select()parameters (lines 1424-1426), the frame merging logic at lines 1467-1471 contains an unprotected edge case:
- Line 1469 checks
i < len(figure_like.frames)to handle charge_state_fig having more frames, but doesn't guard against it having fewer frames- If charge_state has fewer unique values in the
animate_bydimension, the loop terminates early, leaving later animation frames without charge_state tracesAdditionally, this code path is not covered by tests—the test suite explicitly disables
animate_byforplot_charge_state(lines 70-71 intest_results_plots.py).Recommendations:
- Add a defensive check when fewer charge_state frames exist
- Add test coverage for the
animate_bypath withplot_charge_stateto ensure frame alignment behaves as expected
2342-2364: LGTM: Simplified selection logic.The new
_apply_selection_to_datafunction is cleaner than the old indexer-based approach, leveraging xarray's native.sel()method. The suffix generation provides useful context for plot titles.Verify that the old
_apply_indexer_to_datafunction has been completely removed:
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
flixopt/plotting.py (1)
562-579: Set opacity for stacked area traces to prevent hiding layers.The stacked area traces set
fillcolorandline.width = 0but don't specify opacity. This makes areas fully opaque, which can hide underlying layers in the stack and reduce readability.Apply this diff to add translucency:
if cls in ('positive', 'negative'): # Stacked area: add opacity to avoid hiding layers, remove line border if hasattr(trace, 'line') and trace.line.color: trace.fillcolor = trace.line.color trace.line.width = 0 + # Make areas translucent so overlapping stacks remain readable + trace.opacity = 0.6 elif cls == 'mixed':Note: This issue was previously flagged and remains unaddressed.
🧹 Nitpick comments (1)
flixopt/plotting.py (1)
769-773: Consider lowering the log level for auto-reshape decision.The INFO level logging for automatic time reshaping may be too noisy in production environments where this function is called frequently. Since this is an internal decision with sensible defaults, consider using DEBUG level or making it configurable.
- logger.info( + logger.debug( "Auto-applying time reshaping: Only 'time' dimension remains after faceting/animation. " "Using default timeframes='D' and timesteps_per_frame='h'. " "To customize, use reshape_time=('D', 'h') or disable with reshape_time=None." )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(2 hunks)flixopt/plotting.py(4 hunks)tests/test_results_plots.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_results_plots.py
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/plotting.py (2)
flixopt/results.py (2)
size(1589-1597)sizes(433-460)flixopt/flow_system.py (2)
coords(769-775)resample(926-976)
🔇 Additional comments (4)
CHANGELOG.md (1)
57-83: LGTM! Well-structured changelog entries.The changelog entries are clear, well-organized, and follow the project's formatting conventions. The descriptions effectively communicate user-facing changes while maintaining appropriate technical detail.
flixopt/plotting.py (3)
42-42: LGTM! Standard xarray import.The xarray import is necessary for the new xr.DataArray/Dataset input support and follows the standard
xralias convention.
329-525: LGTM! Comprehensive signature expansion with excellent validation.The with_plotly function has been well-extended to support xarray inputs and faceting/animation:
- Proper type hints for the expanded input types
- Defensive empty data checks
- Good UX warning when fig conflicts with faceting
- Clean xarray-to-long-form conversion handling both DataArray and Dataset
- Robust dimension validation with appropriate logging levels
- Clear faceting setup with proper constraints (max 2 dimensions)
1461-1763: LGTM! Well-designed heatmap functions with robust validation.Both heatmap_with_plotly and heatmap_with_matplotlib are well-implemented:
- Comprehensive parameter validation with clear error messages
- Appropriate warnings for backend limitations (e.g., px.imshow facet_col constraint)
- Good dimension validation ensuring at least 2D data for heatmap
- Graceful fallback handling in the Plotly version
- Excellent docstrings with practical examples
- Proper code reuse through reshape_data_for_heatmap helper
The functions are lengthy but remain readable due to clear structure and logical flow.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
flixopt/plotting.py (2)
455-485: Consider warning level for missing dimension messages.Lines 462-465, 472-475, and 481-484 log missing dimensions at debug level. Users might not see these messages in default logging configurations. Consider using
logger.warning()instead so users are informed when their faceting/animation requests cannot be fulfilled.- logger.debug( + logger.warning( f"Dimension '{facet_by}' not found in data. Available dimensions: {available_dims}. " f'Ignoring facet_by parameter.' )
1554-1586: Consider extracting dimension validation into a helper function.The dimension validation logic (lines 1554-1586) is nearly identical to the validation in
with_plotly(lines 455-485). Extracting this into a reusable helper function would reduce code duplication and improve maintainability.Example:
def _validate_facet_dimensions( facet_by: str | list[str] | None, animate_by: str | None, available_dims: list[str] ) -> tuple[str | list[str] | None, str | None]: """Validate and filter facet_by and animate_by dimensions.""" # Existing validation logic here return facet_by, animate_by
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(2 hunks)examples/04_Scenarios/scenario_example.py(5 hunks)flixopt/plotting.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
flixopt/plotting.py (1)
flixopt/flow_system.py (3)
coords(769-775)resample(926-976)isel(889-924)
examples/04_Scenarios/scenario_example.py (6)
flixopt/results.py (4)
flow_system(290-306)size(1589-1597)plot_charge_state(1347-1512)plot_node_balance_pie(1108-1254)flixopt/flow_system.py (1)
FlowSystem(46-980)flixopt/effects.py (1)
Effect(32-387)flixopt/linear_converters.py (10)
Boiler(24-103)eta(97-98)eta(101-103)eta(185-186)eta(189-191)CHP(373-482)eta_th(467-468)eta_th(471-473)eta_el(476-477)eta_el(480-482)flixopt/elements.py (1)
Flow(234-494)flixopt/components.py (2)
Storage(241-529)Source(1115-1219)
🔇 Additional comments (15)
examples/04_Scenarios/scenario_example.py (6)
11-12: LGTM: Timesteps extended to one week.The time horizon extension from an unspecified period to one week (168 hours) provides a more realistic demonstration of the scenario capabilities.
17-38: LGTM: Realistic heat demand patterns with proper variation.The heat demand generation uses realistic daily patterns with appropriate random variation (±5% for base case, ±7% for high demand). The tiling and slicing logic correctly handles the week-long simulation period.
80-84: LGTM: Scenario weights properly configured.The scenario weights (60% Base Case, 40% High Demand) are properly structured and sum to 1.0 as expected for probability weights.
98-132: LGTM: Component parameters are realistic and valid.The updated efficiency values (Boiler η=0.92, CHP η_th=0.48, η_el=0.40) are realistic for modern equipment. The CHP total efficiency (0.88) correctly satisfies the constraint η_th + η_el ≤ 1.0.
160-176: LGTM: Realistic gas pricing and CO2 emissions.The gas prices (0.04, 0.06, 0.11 €/kWh for 2020-2022) realistically reflect the energy crisis escalation. The CO2 emissions factor (0.202 kg/kWh) accurately represents natural gas combustion.
204-205: LGTM: Correct usage of updated plotting API.The plotting calls correctly use the new
selectparameter (line 205) replacing the deprecatedindexerparameter, and leverage the default faceting behavior for charge state plotting.CHANGELOG.md (5)
57-60: LGTM: Feature additions accurately documented.The changelog entries correctly describe the new faceting, animation, selection, and styling features added to the plotting module.
65-67: LGTM: Behavior changes clearly documented.The documentation correctly describes the change from implicit auto-selection to explicit
selectparameter usage, which is a notable change in default behavior.
70-70: LGTM: Deprecation properly documented.The deprecation of the
indexerparameter in favor ofselectis clearly stated, giving users notice to update their code.
75-75: LGTM: Bug fix documented.The error handling improvement in
plot_heatmap()is noted for users experiencing issues.
84-84: LGTM: Internal refactoring documented.The function rename is properly documented in the development section for contributors who may reference internal APIs.
flixopt/plotting.py (4)
539-579: LGTM: Area classification and styling logic is sound.The variable classification correctly distinguishes between positive-only, negative-only, and mixed (both signs) variables. Only pure positive/negative variables are stacked, while mixed variables are shown as dashed lines to avoid visual confusion. The styling (fillcolor, line width, dash pattern) appropriately differentiates between these cases.
692-861: LGTM: reshape_data_for_heatmap implements robust time reshaping logic.The function correctly:
- Handles auto-detection of when reshaping is needed
- Validates datetime types and format combinations
- Performs efficient resampling with numpy
- Creates properly formatted labels with special handling for week patterns
- Unstacks data into a 2D heatmap-ready format
The logic is well-structured with appropriate error handling and warnings.
1461-1653: LGTM: heatmap_with_plotly implements robust heatmap creation with faceting.The function correctly:
- Delegates time reshaping to reshape_data_for_heatmap
- Validates dimensions with appropriate logging
- Handles px.imshow's single-facet limitation gracefully
- Provides fallback for error cases
The try/except with fallback ensures users always get output even if advanced features fail.
1655-1764: LGTM: heatmap_with_matplotlib handles 2D limitation appropriately.The function correctly:
- Delegates time reshaping to the unified helper
- Handles >2 dimensions by selecting first slice with clear warning
- Creates proper matplotlib heatmap with colorbar
- Suggests plotly engine for advanced features
The limitation handling is transparent and helpful for users.
| # Power prices: hourly factors (night low, peak high) and period escalation (2020-2022) | ||
| hourly_price_factors = np.array( | ||
| [ | ||
| 0.70, | ||
| 0.65, | ||
| 0.62, | ||
| 0.60, | ||
| 0.62, | ||
| 0.70, | ||
| 0.95, | ||
| 1.15, | ||
| 1.30, | ||
| 1.25, | ||
| 1.10, | ||
| 1.00, | ||
| 0.95, | ||
| 0.90, | ||
| 0.88, | ||
| 0.92, | ||
| 1.00, | ||
| 1.10, | ||
| 1.25, | ||
| 1.40, | ||
| 1.35, | ||
| 1.20, | ||
| 0.95, | ||
| 0.80, | ||
| ] | ||
| ) | ||
| power_prices = np.array([0.08, 0.09, 0.10]) | ||
| period_base_prices = np.array([0.075, 0.095, 0.135]) # €/kWh for 2020, 2021, 2022 | ||
|
|
||
| flow_system = fx.FlowSystem(timesteps=timesteps, periods=periods, scenarios=scenarios, weights=np.array([0.5, 0.6])) | ||
| price_series = np.zeros((n_hours, 3)) | ||
| for period_idx, base_price in enumerate(period_base_prices): | ||
| price_series[:, period_idx] = ( | ||
| np.tile(hourly_price_factors, n_hours // 24 + 1)[:n_hours] | ||
| * base_price | ||
| * (1 + np.random.uniform(-0.03, 0.03, n_hours)) | ||
| ) | ||
|
|
||
| power_prices = price_series.mean(axis=0) |
There was a problem hiding this comment.
Fix dimensional error in power price calculation.
Line 78 takes mean(axis=0), which averages across time (axis 0) and produces 3 scalar values (one per period). However, power_prices is later used in effects_per_flow_hour (line 181), which requires temporal data varying over time.
This collapses the hourly price variation into period averages, losing all time-of-day pricing information.
Apply this diff to preserve time-varying prices:
- power_prices = price_series.mean(axis=0)
+ # power_prices shape: (n_hours, 3) for periods [2020, 2021, 2022]
+ power_prices = price_seriesOr if period-specific prices are needed, construct a proper time series per period using the FlowSystem's period dimension.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Power prices: hourly factors (night low, peak high) and period escalation (2020-2022) | |
| hourly_price_factors = np.array( | |
| [ | |
| 0.70, | |
| 0.65, | |
| 0.62, | |
| 0.60, | |
| 0.62, | |
| 0.70, | |
| 0.95, | |
| 1.15, | |
| 1.30, | |
| 1.25, | |
| 1.10, | |
| 1.00, | |
| 0.95, | |
| 0.90, | |
| 0.88, | |
| 0.92, | |
| 1.00, | |
| 1.10, | |
| 1.25, | |
| 1.40, | |
| 1.35, | |
| 1.20, | |
| 0.95, | |
| 0.80, | |
| ] | |
| ) | |
| power_prices = np.array([0.08, 0.09, 0.10]) | |
| period_base_prices = np.array([0.075, 0.095, 0.135]) # €/kWh for 2020, 2021, 2022 | |
| flow_system = fx.FlowSystem(timesteps=timesteps, periods=periods, scenarios=scenarios, weights=np.array([0.5, 0.6])) | |
| price_series = np.zeros((n_hours, 3)) | |
| for period_idx, base_price in enumerate(period_base_prices): | |
| price_series[:, period_idx] = ( | |
| np.tile(hourly_price_factors, n_hours // 24 + 1)[:n_hours] | |
| * base_price | |
| * (1 + np.random.uniform(-0.03, 0.03, n_hours)) | |
| ) | |
| power_prices = price_series.mean(axis=0) | |
| # Power prices: hourly factors (night low, peak high) and period escalation (2020-2022) | |
| hourly_price_factors = np.array( | |
| [ | |
| 0.70, | |
| 0.65, | |
| 0.62, | |
| 0.60, | |
| 0.62, | |
| 0.70, | |
| 0.95, | |
| 1.15, | |
| 1.30, | |
| 1.25, | |
| 1.10, | |
| 1.00, | |
| 0.95, | |
| 0.90, | |
| 0.88, | |
| 0.92, | |
| 1.00, | |
| 1.10, | |
| 1.25, | |
| 1.40, | |
| 1.35, | |
| 1.20, | |
| 0.95, | |
| 0.80, | |
| ] | |
| ) | |
| period_base_prices = np.array([0.075, 0.095, 0.135]) # €/kWh for 2020, 2021, 2022 | |
| price_series = np.zeros((n_hours, 3)) | |
| for period_idx, base_price in enumerate(period_base_prices): | |
| price_series[:, period_idx] = ( | |
| np.tile(hourly_price_factors, n_hours // 24 + 1)[:n_hours] | |
| * base_price | |
| * (1 + np.random.uniform(-0.03, 0.03, n_hours)) | |
| ) | |
| # power_prices shape: (n_hours, 3) for periods [2020, 2021, 2022] | |
| power_prices = price_series |
🤖 Prompt for AI Agents
In examples/04_Scenarios/scenario_example.py around lines 39-78, the code
collapses the hourly price_series to 3 period averages by calling
price_series.mean(axis=0); this removes hourly variation required later by
effects_per_flow_hour. Preserve the time-varying series instead: keep
power_prices as the full price_series (n_hours x periods) so downstream code
receives per-hour prices, or if a single hourly vector is required choose the
appropriate period column (e.g., price_series[:, period_index]) matched to the
FlowSystem period dimension; update any downstream calls that expect the old
shape accordingly.
| relative_maximum_final_charge_state=np.array([0.8, 0.5, 0.1]), | ||
| eta_charge=0.95, # Realistic charging efficiency (~95%) | ||
| eta_discharge=0.98, # Realistic discharging efficiency (~98%) | ||
| relative_loss_per_hour=np.array([0.008, 0.015]), # Realistic thermal losses: 0.8-1.5% per hour |
There was a problem hiding this comment.
🧩 Analysis chain
Verify dimensional alignment of storage parameter arrays.
The arrays have dimensions that suggest specific mappings:
relative_maximum_final_charge_state(3 values) likely maps to 3 periodsrelative_loss_per_hour(2 values) likely maps to 2 scenarios
Ensure this dimensional mapping is intentional and aligns with how the FlowSystem will interpret these parameters.
Run the following script to confirm the dimensional interpretation:
🏁 Script executed:
#!/bin/bash
# Verify how Storage parameters handle array inputs and their dimensional mapping
# Check Storage class parameter documentation
ast-grep --pattern $'class Storage($$$):
$$$
def __init__(
$$$,
relative_maximum_final_charge_state$$$,
$$$,
relative_loss_per_hour$$$,
$$$
):
$$$'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find Storage class definition
echo "=== Searching for Storage class ==="
rg -n "class Storage" --type=py -A 20
echo -e "\n=== Searching for relative_maximum_final_charge_state in Storage context ==="
rg -n "relative_maximum_final_charge_state" --type=py -B 5 -A 2
echo -e "\n=== Searching for relative_loss_per_hour in Storage context ==="
rg -n "relative_loss_per_hour" --type=py -B 5 -A 2Length of output: 27465
🏁 Script executed:
#!/bin/bash
# Verify periods and scenarios count in scenario_example.py
echo "=== Searching for periods definition in scenario_example.py ==="
rg -n "periods|scenarios" examples/04_Scenarios/scenario_example.py -B 2 -A 1 | head -40
echo -e "\n=== Getting FlowSystem setup context ==="
rg -n "FlowSystem" examples/04_Scenarios/scenario_example.py -A 15 | head -50Length of output: 1603
🏁 Script executed:
#!/bin/bash
# Find fit_to_model_coords implementation to understand array handling
echo "=== Searching for fit_to_model_coords definition ==="
rg -n "def fit_to_model_coords" --type=py -A 30
echo -e "\n=== Check for any validation of array shapes ==="
rg -n "relative_loss_per_hour|relative_maximum_final_charge_state" flixopt/components.py -B 2 -A 2 | grep -A 5 "fit_to_model_coords"Length of output: 2808
Fix array dimensions to match expected parameter shapes in Storage component.
The storage parameters have dimensional mismatches that will cause failures or unexpected behavior:
-
relative_maximum_final_charge_state=np.array([0.8, 0.5, 0.1])— Provided 3 values but the framework expects shape matchingdims=['period', 'scenario'](i.e., 3×2=6 values or a scalar). The 3 values align only with periods, missing the scenario dimension. -
relative_loss_per_hour=np.array([0.008, 0.015])— Provided 2 values for aTemporalDataUserparameter that defaults to all model dimensions (time, period, scenario). Expected shape is 168×3×2 or scalar. The 2 values only match scenarios.
Solutions:
- Use scalar values:
relative_maximum_final_charge_state=0.8andrelative_loss_per_hour=0.008 - Or reshape arrays to match full expected dimensions with proper broadcasting semantics
🤖 Prompt for AI Agents
In examples/04_Scenarios/scenario_example.py around lines 146 to 149, the
Storage parameters use arrays with incorrect dimensions:
relative_maximum_final_charge_state is a length-3 array (periods) but the
Storage expects dims=['period','scenario'] (3×2) or a scalar, and
relative_loss_per_hour is length-2 (scenarios) but TemporalDataUser expects full
time×period×scenario shape (168×3×2) or a scalar; fix by replacing these with
scalars (e.g., relative_maximum_final_charge_state=0.8 and
relative_loss_per_hour=0.008) or construct arrays with the full expected shape
(or broadcastable shape) matching model dimensions (time, period, scenario) so
shapes align with the Storage component requirements.
| # Convert xarray to long-form DataFrame for Plotly Express | ||
| if isinstance(data, (xr.DataArray, xr.Dataset)): | ||
| # Convert to long-form (tidy) DataFrame | ||
| # Structure: time, variable, value, scenario, period, ... (all dims as columns) | ||
| if isinstance(data, xr.Dataset): | ||
| # Stack all data variables into long format | ||
| df_long = data.to_dataframe().reset_index() | ||
| # Melt to get: time, scenario, period, ..., variable, value | ||
| id_vars = [dim for dim in data.dims] | ||
| value_vars = list(data.data_vars) | ||
| df_long = df_long.melt(id_vars=id_vars, value_vars=value_vars, var_name='variable', value_name='value') | ||
| else: | ||
| # DataArray | ||
| df_long = data.to_dataframe().reset_index() | ||
| if data.name: | ||
| df_long = df_long.rename(columns={data.name: 'value'}) | ||
| else: | ||
| # Unnamed DataArray, find the value column | ||
| non_dim_cols = [col for col in df_long.columns if col not in data.dims] | ||
| if len(non_dim_cols) != 1: | ||
| raise ValueError( | ||
| f'Expected exactly one non-dimension column for unnamed DataArray, ' | ||
| f'but found {len(non_dim_cols)}: {non_dim_cols}' | ||
| ) | ||
| value_col = non_dim_cols[0] | ||
| df_long = df_long.rename(columns={value_col: 'value'}) | ||
| df_long['variable'] = data.name or 'data' | ||
| else: | ||
| # Already a DataFrame - convert to long format for Plotly Express | ||
| df_long = data.reset_index() | ||
| if 'time' not in df_long.columns: | ||
| # First column is probably time | ||
| df_long = df_long.rename(columns={df_long.columns[0]: 'time'}) | ||
| # Melt to long format | ||
| id_vars = [ | ||
| col | ||
| for col in df_long.columns | ||
| if col in ['time', 'scenario', 'period'] | ||
| or col in (facet_by if isinstance(facet_by, list) else [facet_by] if facet_by else []) | ||
| ] | ||
| value_vars = [col for col in df_long.columns if col not in id_vars] | ||
| df_long = df_long.melt(id_vars=id_vars, value_vars=value_vars, var_name='variable', value_name='value') |
There was a problem hiding this comment.
Consider edge cases in DataFrame dimension detection.
Lines 446-452 attempt to identify dimension columns for DataFrame inputs by checking for known names ('time', 'scenario', 'period') and facet_by dimensions. This heuristic may incorrectly classify columns for DataFrames with:
- Custom dimension names (e.g., 'location', 'technology')
- Columns named like dimensions but containing values
This is acceptable for typical usage but could cause issues with non-standard DataFrame structures.
Consider documenting the expected DataFrame structure or adding a parameter to explicitly specify dimension columns.
🤖 Prompt for AI Agents
In flixopt/plotting.py around lines 412-453, the heuristic that picks id_vars
for DataFrame inputs (currently only looking for 'time','scenario','period' and
facet_by) is brittle for non-standard DataFrames; add an optional parameter
(e.g., dims or id_vars) to the plotting function so callers can explicitly pass
the columns to treat as dimension/index columns, and update the logic to: use
the provided dims/id_vars if given; otherwise fall back to the current heuristic
(ensure facet_by is normalized to a list first), and if the heuristic finds
nothing, default to treating the first column as 'time' and warn (or raise) to
prompt explicit dims. Also update the docstring to describe the expected
DataFrame shape and the new parameter so users with custom dimension names can
opt in.
…ceting-and-animations # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
67-67: Make the scenario_example.py improvements more specific.Line 67 states "Improved
scenario_example.py" but lacks detail. The PR objectives mention "Adjusts frame size; removes height-related code" and other changes. Provide concrete examples of what was improved so users understand the changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(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.13)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (2)
CHANGELOG.md (2)
65-65: Clarify whether the selection behavior change qualifies as a breaking change.Line 65 states that "no longer automatically selects first value for non-time dimensions" — a behavioral change that could break existing code relying on the old auto-selection behavior. This appears to be listed under "Changed" rather than "Breaking Changes" (line 62). Verify this categorization is intentional and will not surprise users upgrading.
73-73: Document removals in the "Removed" section.The PR objectives mention "removes height-related code and removes an old class," but the "Removed" section (line 73) is empty. If these are user-facing or could impact existing code, document them explicitly here. If they are internal-only, consider mentioning in the Development section instead.
Add animation and faceting options to plots
Adjust size of the frame
Utilize plotly express directly
Rmeocve old class
Use plotly express and modify stackgroup afterwards
Add modifications also to animations
Mkae more compact
Remove height stuff
Remove line and make set opacity =0 for area
Integrate faceting and animating into existing with_plotly method
Improve results.py
Improve results.py
Move check if dims are found to plotting.py
Fix usage of indexer
Change selection string with indexer
Change behaviout of parameter "indexing"
Update CHANGELOG.md
Add new selection parameter to plotting methods
deprectae old indexer parameter
deprectae old indexer parameter
Add test
Add test
Add test
Add test
Fix not supportet check for matplotlib
Typo in CHANGELOG.md
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
Changed
Deprecated
Tests / Docs
Examples