Feature/plotting dataset first #438
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
* 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
* 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
…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
# Conflicts: # CHANGELOG.md
WalkthroughPlotting was refactored to be xarray Dataset–centric: added dataset normalization/validation and color-resolution helpers, expanded Plotly/Matplotlib APIs with passthrough kwargs, updated results/aggregation and examples to use datasets, and added tests for validation and kwargs forwarding. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PlottingLayer as Plotting Layer
participant Ensure as _ensure_dataset
participant Validate as _validate_plotting_data
participant Colors as resolve_colors
participant Backend as Plotly/Matplotlib
User->>PlottingLayer: with_plotly(data (DataFrame|Dataset), colors, **plot_kwargs)
PlottingLayer->>Ensure: normalize input -> xr.Dataset
Ensure-->>PlottingLayer: xr.Dataset (or raise)
PlottingLayer->>Validate: check numeric/dims/nan
Validate-->>PlottingLayer: pass / warnings / raise
PlottingLayer->>Colors: map variables -> colors dict
Colors-->>PlottingLayer: dict[name→color]
PlottingLayer->>Backend: render traces + trace_kwargs/layout_kwargs/imshow_kwargs
Backend-->>PlottingLayer: Figure
PlottingLayer-->>User: Figure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
flixopt/aggregation.py (1)
145-145: Ensure CONFIG default is actually reachable; and keep colors consistent between Original vs Aggregated.
- With default
colormap='viridis', the fallback toCONFIG.Plotting.default_qualitative_colorscaleis never used. Makecolormapoptional.- Today, “Original - X” and “Aggregated - X” are colored independently, which can misalign comparisons.
- def plot(self, colormap: str = 'viridis', show: bool = True, save: pathlib.Path | None = None) -> go.Figure: + def plot(self, colormap: str | None = None, show: bool = True, save: pathlib.Path | None = None) -> go.Figure: @@ - fig = plotting.with_plotly( - df_org.to_xarray(), - 'line', - colors=colormap or CONFIG.Plotting.default_qualitative_colorscale, - xlabel='Time in h', - ) + fig = plotting.with_plotly( + df_org.to_xarray(), + 'line', + colors=colormap or CONFIG.Plotting.default_qualitative_colorscale, + xlabel='Time in h', + ) @@ - fig2 = plotting.with_plotly( - df_agg.to_xarray(), - 'line', - colors=colormap or CONFIG.Plotting.default_qualitative_colorscale, - xlabel='Time in h', - ) + # Use the same color mapping for both figures: map base names to a single color. + base_labels = list(self.original_data.columns) + colorscale = (colormap or CONFIG.Plotting.default_qualitative_colorscale) + # Build a stable mapping Original/Aggregated -> same color by base label + color_map = {f'Original - {b}': colorscale[i % len(colorscale)] for i, b in enumerate(base_labels)} + color_map |= {f'Aggregated - {b}': color_map[f'Original - {b}'] for b in base_labels} + fig2 = plotting.with_plotly( + df_agg.to_xarray(), + 'line', + colors=color_map, + xlabel='Time in h', + )If
plotting.with_plotlysupports a dict color map (see note below), this guarantees identical colors for each base series across Original/Aggregated.Also applies to: 154-159, 162-167, 168-169
flixopt/results.py (1)
1182-1193: Guard unsupportedmode='area'for Matplotlib early.Matplotlib helper only supports 'stacked_bar' and 'line'. Add an early check to fail fast with a clear message.
if engine == 'plotly': figure_like = plotting.with_plotly( @@ else: + if mode == 'area': + raise ValueError('mode="area" is not supported for engine="matplotlib". Use "stacked_bar" or "line".') figure_like = plotting.with_matplotlib(flixopt/plotting.py (1)
1310-1322: dual_pie_with_plotly: lower_percentage_group parameter is unused.Public API advertises grouping small slices, but it’s not implemented here (unlike the Matplotlib variant).
Either implement the grouping (mirroring dual_pie_with_matplotlib.preprocess_series) or remove the parameter from the signature/docs. I can provide a patch if you confirm desired behavior.
Also applies to: 1348-1353
🧹 Nitpick comments (12)
CHANGELOG.md (2)
58-58: Capitalize “Matplotlib” for consistency and consider end punctuation.Use “Matplotlib” (proper noun) consistently and add a trailing period for parallelism with nearby entries.
- - **Template integration**: Plotly templates now fully control plot styling without hardcoded overrides + - **Template integration**: Plotly templates now fully control plot styling without hardcoded overrides.
63-64: Style: capitalize “Matplotlib”.Match casing used elsewhere in the docs.
- - Improved error messages for matplotlib with multidimensional data + - Improved error messages for Matplotlib with multidimensional dataexamples/02_Complex/complex_example_results.py (1)
28-30: Avoid opening many figures in a loop; save or disable show.Looping over all buses with default show=True can spawn many windows. Prefer saving or explicitly
show=False.- for bus in results.buses.values(): - bus.plot_node_balance_pie() - bus.plot_node_balance() + for bus in results.buses.values(): + bus.plot_node_balance_pie(show=False, save=True) + bus.plot_node_balance(show=False, save=True)examples/03_Calculation_types/example_calculation_types.py (1)
205-210: Usegrouped_barmode instead of overriding stacked layout.You set
mode='stacked_bar'and then switch to grouped via layout. Use the dedicated mode for clarity and correctness.- fx.plotting.with_plotly( - get_solutions(calculations, 'costs(temporal)|per_timestep').sum('time'), - mode='stacked_bar', - title='Total Cost Comparison', - ylabel='Costs [€]', - ).update_layout(barmode='group').write_html('results/Total Costs.html') + fx.plotting.with_plotly( + get_solutions(calculations, 'costs(temporal)|per_timestep').sum('time'), + mode='grouped_bar', + title='Total Cost Comparison', + ylabel='Costs [€]', + ).write_html('results/Total Costs.html')Please skim the rendered chart to confirm bar grouping matches expectations for all calculations.
Also applies to: 212-217, 219-224, 226-231, 233-236
tests/test_plotting_api.py (1)
35-45: Add a test for dict-based color mapping and px kwargs passthrough.Current tests don’t verify that
colorsworks when passed as a dict, or thatpx_kwargsare forwarded. Add two small tests.def test_colors_dict_mapping_plotly(sample_dataset): color_map = {'var1': '#ff0000', 'var2': '#00ff00', 'var3': '#0000ff'} fig = plotting.with_plotly(sample_dataset, mode='line', colors=color_map) # All traces should use the provided colors trace_colors = {t.name: t.line.color if hasattr(t, 'line') else getattr(t, 'marker', {}).get('color') for t in fig.data} for var, expected in color_map.items(): assert any(c == expected for n, c in trace_colors.items() if n == var) def test_px_kwargs_forwarded(sample_dataset): fig = plotting.with_plotly(sample_dataset, mode='line', facet_by=None, animate_by=None, template='plotly_dark') assert fig.layout.template == 'plotly_dark'Run the tests after adding these to ensure the new plotting surface behaves as expected.
flixopt/results.py (2)
1196-1203: Matplotlib path: addxlabel='Time in h'for parity with Plotly.Keeps axis labeling consistent across engines.
- figure_like = plotting.with_matplotlib( - ds, - colors=resolved_colors, - mode=mode, - title=title, - **plot_kwargs, - ) + figure_like = plotting.with_matplotlib( + ds, + colors=resolved_colors, + mode=mode, + title=title, + xlabel='Time in h', + **plot_kwargs, + )
2249-2255: Error message grammar.Minor polish for the Matplotlib-heatmap error string.
- f'Use select={{...}} to reduce to time only, use "reshape_time=None" or switch to engine="plotly" or use for multi-dimensional support.' + f'Use select={{...}} to reduce to time only, use "reshape_time=None", or switch to engine="plotly" for multi-dimensional support.'flixopt/plotting.py (5)
373-392: resolve_colors typing and robustness.Type says dict[str, str], but Matplotlib may return RGBA tuples; also keep only labels present and log extras.
-def resolve_colors( +def resolve_colors( data: xr.Dataset, colors: ColorType, engine: PlottingEngine = 'plotly', -) -> dict[str, str]: +) -> dict[str, Any]: @@ - if isinstance(colors, dict): - return colors + if isinstance(colors, dict): + extra = set(colors) - set(labels) + if extra: + logger.debug(f'Ignoring extra color labels not in data: {sorted(extra)}') + return {k: v for k, v in colors.items() if k in labels}
576-580: Prefer resolve_colors here for consistency.You already have resolve_colors; using it avoids duplicate logic.
- processed_colors = ColorProcessor(engine='plotly').process_colors(colors, all_vars) - color_discrete_map = {var: color for var, color in zip(all_vars, processed_colors, strict=True)} + color_discrete_map = resolve_colors(_ensure_dataset(data), colors, engine='plotly')
331-340: _ensure_dataset: consider xr.DataArray and pd.Series support.Many call sites may naturally hold a DataArray/Series. Converting those to a single‑var Dataset improves ergonomics.
-def _ensure_dataset(data: xr.Dataset | pd.DataFrame) -> xr.Dataset: +def _ensure_dataset(data: xr.Dataset | xr.DataArray | pd.DataFrame | pd.Series) -> xr.Dataset: @@ - elif isinstance(data, pd.DataFrame): + elif isinstance(data, pd.DataFrame): return data.to_xarray() + elif isinstance(data, xr.DataArray): + return data.to_dataset(name=data.name or 'value') + elif isinstance(data, pd.Series): + return data.to_frame(name=data.name or 'value').to_xarray()
342-355: _validate_plotting_data: simplify zero‑size checks.The branched logic is hard to read and can be reduced.
- if all(data[var].size == 0 for var in data.data_vars) if len(data.data_vars) > 0 else True: - if not allow_empty and len(data.data_vars) > 0: - raise ValueError('Dataset has zero size. Cannot create plot.') - if len(data.data_vars) == 0: - return # Empty dataset, nothing to validate - return + if len(data.data_vars) == 0: + if not allow_empty: + raise ValueError('Empty Dataset provided (no variables). Cannot create plot.') + return + if all(data[var].size == 0 for var in data.data_vars): + if not allow_empty: + raise ValueError('Dataset has zero size. Cannot create plot.') + return
1987-2068: export_figure: good ergonomics; minor polish.Looks solid. Consider documenting that dpi only applies to Matplotlib in the docstring’s Args. Also fix “doesnt” → “doesn’t”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)examples/02_Complex/complex_example_results.py(1 hunks)examples/03_Calculation_types/example_calculation_types.py(1 hunks)flixopt/aggregation.py(2 hunks)flixopt/plotting.py(29 hunks)flixopt/results.py(32 hunks)tests/test_plotting_api.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
examples/03_Calculation_types/example_calculation_types.py (1)
flixopt/plotting.py (1)
with_plotly(394-687)
flixopt/aggregation.py (1)
flixopt/plotting.py (1)
with_plotly(394-687)
examples/02_Complex/complex_example_results.py (2)
flixopt/structure.py (1)
values(1134-1135)flixopt/results.py (2)
plot_node_balance_pie(1215-1392)plot_node_balance(1024-1213)
tests/test_plotting_api.py (1)
flixopt/plotting.py (2)
with_plotly(394-687)_ensure_dataset(331-339)
flixopt/results.py (1)
flixopt/plotting.py (5)
resolve_colors(373-391)with_matplotlib(690-837)dual_pie_with_plotly(1310-1444)dual_pie_with_matplotlib(1447-1628)with_plotly(394-687)
flixopt/plotting.py (2)
flixopt/config.py (1)
CONFIG(61-322)flixopt/aggregation.py (1)
plot(145-184)
🔇 Additional comments (4)
flixopt/results.py (3)
1586-1615: Color consistency when overlaying charge_state.Good approach combining datasets for
resolve_colors. Minor: explicitly reserve black for charge state to avoid accidental remaps if a variable named...|charge_stateexists elsewhere; your explicittrace.line.color = 'black'handles Plotly—consider adding a note or ensuring the mapping excludes this key.Please confirm that for storages with inputs/outputs named similarly to
charge_state, no ambiguity occurs in legends.
2269-2271: DPI passthrough looks good.Extracting
dpifromplot_kwargsand passing toexport_figureis clean and consistent across engines.Also applies to: 2303-2311
1158-1167: Based on the shell script output analysis, the review comment contains an incorrect concern. The current implementation properly handles dictionary color inputs:
resolve_colors()(lines 383-384) explicitly checks ifcolorsis a dict and returns it unchanged—dicts are NOT processed byColorProcessorwith_plotly()(line 489) callsresolve_colors()which respects dict inputs, ensuring pre-resolved mappings are preserved- No double-processing occurs when dicts are passed through the color pipeline
The implementation correctly honors
color_discrete_mapdictionaries throughout the plotting pipeline.Likely an incorrect or invalid review comment.
flixopt/plotting.py (1)
579-580: I need to check the explicitpython_requiresfield inpyproject.tomlto verify the minimum Python version supported.Now let me verify the exact
python_requiresfield in pyproject.toml:Review comment is incorrect — project requires Python 3.10+, making zip(strict=...) appropriate.
The review assumes the project supports Python versions below 3.10, but the pyproject.toml classifiers explicitly list only Python 3.10, 3.11, 3.12, and 3.13. Since the strict parameter was added in Python 3.10, all four usages of
zip(strict=...)are valid for the project's declared minimum version. No changes or guards are necessary.Additionally, the review identifies only 2 occurrences (lines 579–580 and 765), but the script found 4 total:
flixopt/plotting.py:579,flixopt/plotting.py:765,flixopt/results.py:692, andflixopt/calculation.py:560.Likely an incorrect or invalid review comment.
| data: xr.Dataset | pd.DataFrame, | ||
| mode: Literal['stacked_bar', 'line', 'area', 'grouped_bar'] = 'stacked_bar', | ||
| colors: ColorType = 'viridis', | ||
| title: str = '', | ||
| ylabel: str = '', | ||
| xlabel: str = 'Time in h', | ||
| fig: go.Figure | None = None, | ||
| xlabel: str = '', | ||
| facet_by: str | list[str] | None = None, | ||
| animate_by: str | None = None, | ||
| facet_cols: int = 3, | ||
| facet_cols: int | None = None, | ||
| shared_yaxes: bool = True, | ||
| shared_xaxes: bool = True, | ||
| trace_kwargs: dict[str, Any] | None = None, | ||
| layout_kwargs: dict[str, Any] | None = None, | ||
| **px_kwargs: Any, | ||
| ) -> go.Figure: |
There was a problem hiding this comment.
px_kwargs are accepted but never passed to px. — wire them through.*
This breaks the advertised kwargs‑passthrough. Merge px_kwargs into common_args before creating the figure.
Apply this diff:
@@
def with_plotly(
data: xr.Dataset | pd.DataFrame,
@@
- layout_kwargs: dict[str, Any] | None = None,
- **px_kwargs: Any,
+ layout_kwargs: dict[str, Any] | None = None,
+ **px_kwargs: Any,
) -> go.Figure:
@@
common_args = {
'data_frame': df_long,
'x': x_dim,
'y': 'value',
'color': 'variable',
'facet_row': facet_row,
'facet_col': facet_col,
'animation_frame': animate_by,
'color_discrete_map': color_discrete_map,
'title': title,
'labels': {'value': ylabel, x_dim: xlabel, 'variable': ''},
}
+ # Let callers override/extend Plotly Express parameters
+ if px_kwargs:
+ common_args.update(px_kwargs)
@@
- if mode == 'stacked_bar':
- fig = px.bar(**common_args)
+ if mode == 'stacked_bar':
+ fig = px.bar(**common_args)
@@
- elif mode == 'grouped_bar':
- fig = px.bar(**common_args)
+ elif mode == 'grouped_bar':
+ fig = px.bar(**common_args)
@@
- elif mode == 'line':
- fig = px.line(**common_args, line_shape='hv') # Stepped lines
+ elif mode == 'line':
+ fig = px.line(**common_args, line_shape=common_args.pop('line_shape', 'hv')) # preserve override
@@
- elif mode == 'area':
- # Use Plotly Express to create the area plot (preserves animation, legends, faceting)
- fig = px.area(**common_args, line_shape='hv')
+ elif mode == 'area':
+ # Use Plotly Express to create the area plot (preserves animation, legends, faceting)
+ fig = px.area(**common_args, line_shape=common_args.pop('line_shape', 'hv'))Also applies to: 621-633
🤖 Prompt for AI Agents
In flixopt/plotting.py around lines 395 to 409 (and similarly around 621 to
633), px_kwargs are accepted by the function but never forwarded to the
plotly.express creator; merge px_kwargs into the common_args (or equivalent dict
used to call px.*) before calling the px function so those extra keyword
arguments are passed through, e.g., update/extend common_args with px_kwargs
(handling None) and ensure no keys are overwritten unintentionally, then proceed
to create the figure using the merged dict.
| if len(data.data_vars) == 0: | ||
| logger.error('"with_plotly() got an empty Dataset.') | ||
| return go.Figure() |
There was a problem hiding this comment.
Stray quote in error log message.
Minor typo; the leading double‑quote leaks into logs.
- logger.error('"with_plotly() got an empty Dataset.')
+ logger.error('with_plotly() got an empty Dataset.')📝 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.
| if len(data.data_vars) == 0: | |
| logger.error('"with_plotly() got an empty Dataset.') | |
| return go.Figure() | |
| if len(data.data_vars) == 0: | |
| logger.error('with_plotly() got an empty Dataset.') | |
| return go.Figure() |
🤖 Prompt for AI Agents
In flixopt/plotting.py around lines 477 to 479, the error log message contains a
stray leading double-quote; remove the extra quote so the log reads:
with_plotly() got an empty Dataset. Update the logger.error call to use the
corrected string (no leading quote) and keep the return go.Figure() behavior
unchanged.
| df = data.to_dataframe() | ||
|
|
||
| # Get colors in column order | ||
| processed_colors = [color_discrete_map.get(str(col), '#808080') for col in df.columns] | ||
|
|
||
| if mode == 'stacked_bar': | ||
| cumulative_positive = np.zeros(len(data)) | ||
| cumulative_negative = np.zeros(len(data)) | ||
| width = data.index.to_series().diff().dropna().min() # Minimum time difference | ||
| cumulative_positive = np.zeros(len(df)) | ||
| cumulative_negative = np.zeros(len(df)) | ||
| width = df.index.to_series().diff().dropna().min() # Minimum time difference | ||
|
|
There was a problem hiding this comment.
Matplotlib path assumes a 1D numeric/datetime index; MultiIndex/categorical x breaks bar width and plotting.
data.to_dataframe() often yields a MultiIndex. width from index.diff() and ax.bar on a MultiIndex will fail or mis‑size.
Refactor to ensure a 1D index (prefer 'time' if present) and use a safe width fallback:
- # Convert Dataset to DataFrame for matplotlib plotting (naturally wide-form)
- df = data.to_dataframe()
+ # Convert Dataset to DataFrame; ensure a 1D index for plotting
+ df = data.to_dataframe()
+ if isinstance(df.index, pd.MultiIndex):
+ if 'time' in df.index.names:
+ df = df.reset_index().set_index('time').sort_index()
+ else:
+ # fall back to the first level
+ df = df.reset_index().set_index(df.index.names[0]).sort_index()
@@
- width = df.index.to_series().diff().dropna().min() # Minimum time difference
+ # Robust bar width: compute from index spacing when possible, else default
+ try:
+ width = df.index.to_series().diff().dropna().min()
+ if pd.isna(width):
+ width = 0.8
+ except Exception:
+ width = 0.8 # categorical or irregular index
@@
- for i, column in enumerate(df.columns):
- ax.step(df.index, df[column], where='post', color=processed_colors[i], label=column, **plot_kwargs)
+ for i, column in enumerate(df.columns):
+ ax.step(df.index, df[column], where='post', color=processed_colors[i], label=column, **plot_kwargs)If you’d rather hard‑fail on unsupported indexes, raise a ValueError with guidance instead.
Also applies to: 792-817, 821-823
🤖 Prompt for AI Agents
In flixopt/plotting.py around lines 782-791 (also apply same changes to 792-817
and 821-823), the code assumes data.to_dataframe() returns a 1D numeric/datetime
index and uses index.diff() to compute bar width which breaks for MultiIndex or
categorical indexes; update the logic to first convert the DataFrame to a safe
1D x-axis: if the DataFrame has a column named 'time' use that as the x values
(and set it as the index or use it directly), else if the index is a simple
DatetimeIndex or numeric Index accept it, otherwise either compute a safe
default width (e.g., 1.0 or (max(x)-min(x))/len(x) with a guard) and convert the
index to a positional integer range for plotting, or raise a ValueError with a
clear message instructing callers to supply a 1D numeric/datetime index or a
'time' column; ensure subsequent cumulative and ax.bar calls use the chosen x
values and the safe width.
| Configure automatic color management for plots: | ||
| ```python | ||
| # Dict-based configuration: | ||
| results.setup_colors({'Solar*': 'Oranges', 'Wind*': 'Blues', 'Battery': 'green'}) | ||
| # All plots automatically use configured colors (colors=None is the default) | ||
| results['ElectricityBus'].plot_node_balance() | ||
| results['Battery'].plot_charge_state() | ||
| # Override when needed | ||
| results['ElectricityBus'].plot_node_balance(colors='turbo') # Ignores setup | ||
| ``` | ||
There was a problem hiding this comment.
🧩 Analysis chain
Docstring mentions results.setup_colors(...); verify API exists.
I don’t see setup_colors in this file. If it’s defined elsewhere, fine; otherwise, please adjust or add it to avoid broken docs.
🏁 Script executed:
#!/bin/bash
# Search for a setup_colors API across the repo
rg -nP --type=py -C2 '\bsetup_colors\s*\('Length of output: 344
I need to verify if setup_colors is defined elsewhere in the codebase, potentially as a method in the Results class or inherited from a parent class.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for Results class definition and methods, check for setup_colors
rg -nP --type=py -A 100 'class Results' | head -150
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for setup_colors without the call pattern to catch definitions/assignments
rg -nP --type=py 'setup_colors' --type=pyLength of output: 112
🏁 Script executed:
#!/bin/bash
# Check what Results class inherits from and look for all method definitions
ast-grep --pattern $'class Results {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any methods or properties starting with setup or containing colors
rg -nP --type=py 'def\s+(setup|.*colors)' | head -30Length of output: 586
🏁 Script executed:
#!/bin/bash
# Check the file structure and imports at the top of results.py
head -50 flixopt/results.pyLength of output: 1515
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for CalculationResults class definition and any parent classes
rg -nP --type=py 'class CalculationResults' -A 5
Length of output: 756
🏁 Script executed:
#!/bin/bash
# Check if there's a Results alias or any dynamic method creation
rg -nP --type=py '(Results\s*=|def __getattr__|setattr.*setup_colors)'Length of output: 75
🏁 Script executed:
#!/bin/bash
# Look for any setup methods in the CalculationResults class
rg -nP --type=py 'def setup' flixopt/results.py -C 2Length of output: 0
Remove the setup_colors() example from the docstring or implement the method.
The docstring at lines 112–123 documents a setup_colors() API that does not exist in the CalculationResults class. Searching the entire codebase confirms no implementation of this method. Users following the documented example will encounter an AttributeError at runtime. Either implement this method or replace the example with working code that reflects the actual API.
🤖 Prompt for AI Agents
In flixopt/results.py around lines 110 to 123, the docstring shows a
non-existent setup_colors() API; remove that example and replace it with a
working snippet that uses the actual plotting API (pass colors explicitly to
plot methods or rely on default colors=None). Edit the docstring to delete the
setup_colors() block and replace it with a short example like: configure colors
by passing the colors argument to individual plot calls (e.g.
results['ElectricityBus'].plot_node_balance(colors={'Solar*':'Oranges','Wind*':'Blues'})
or use colors='turbo' to override), and ensure the docstring text explains that
there is no global setup_colors() method.
| data_left=inputs.to_pandas(), | ||
| data_right=outputs.to_pandas(), | ||
| colors=colors, | ||
| colors=resolved_colors, | ||
| title=title, | ||
| subtitles=('Inputs', 'Outputs'), | ||
| legend_title='Flows', | ||
| lower_percentage_group=lower_percentage_group, | ||
| **plot_kwargs, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Provide Series to Matplotlib dual-pie for robustness.
Ensure pd.Series is passed (safer than relying on Dataset.to_pandas() behavior for 0-D vars).
- figure_like = plotting.dual_pie_with_matplotlib(
- data_left=inputs.to_pandas(),
- data_right=outputs.to_pandas(),
+ figure_like = plotting.dual_pie_with_matplotlib(
+ data_left=inputs.to_array().to_pandas(),
+ data_right=outputs.to_array().to_pandas(),
colors=resolved_colors,
title=title,
subtitles=('Inputs', 'Outputs'),
legend_title='Flows',
lower_percentage_group=lower_percentage_group,
**plot_kwargs,
)🤖 Prompt for AI Agents
In flixopt/results.py around lines 1371 to 1379, the call passes
inputs.to_pandas() and outputs.to_pandas() directly to the dual-pie plot which
can produce 0-D scalars; convert those DataFrame/ndarray results into
pandas.Series first to guarantee 1-D data. Specifically, coerce each to a
pd.Series (or use .squeeze() then wrap with pd.Series if needed), preserve the
index/labels, and pass the resulting Series objects as data_left and data_right
to the plotting function; update any variable names used in the call and ensure
downstream plot_kwargs still apply.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
CHANGELOG.md (1)
60-61: Clarify kwargs support if it's a notable feature.The entries on lines 60–61 cover Dataset-first plotting well. However, the PR objectives emphasize "passing kwargs to underlying plotting methods" as a primary goal. Line 60's mention of Plotly templates controlling styling likely relates to this (enabling kwargs passthrough), but it may benefit from more explicit framing if kwargs support is a significant new capability.
Consider whether an entry like "Plotting methods now forward layout and trace kwargs to Plotly" (or equivalent for other backends) would better surface this feature to users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)flixopt/aggregation.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/aggregation.py (1)
flixopt/plotting.py (1)
with_plotly(394-687)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (2)
CHANGELOG.md (1)
68-69: Data validation improvements are well documented.These entries clearly communicate the validation and error-handling improvements, which align well with the PR objectives. The phrasing is user-friendly and specific (e.g., "multidimensional data,"
plot_heatmap()).flixopt/aggregation.py (1)
157-158: LGTM! Trace merging logic is correct.The pattern of iterating over
fig2.dataand adding each trace tofigis a standard and correct approach for combining Plotly figures. This properly preserves the dash styling applied to the original traces while adding the aggregated traces.
| columns={col: f'Aggregated - {col}' for col in self.aggregated_data.columns} | ||
| ) | ||
| fig = plotting.with_plotly(df_org, 'line', colors=colormap) | ||
| fig = plotting.with_plotly(df_org.to_xarray(), 'line', colors=colormap, xlabel='Time in h') |
There was a problem hiding this comment.
🧩 Analysis chain
Verify DataFrame index structure for xarray conversion.
The explicit to_xarray() calls convert pandas DataFrames to xarray Datasets before passing them to plotting.with_plotly. Since with_plotly accepts both xr.Dataset | pd.DataFrame and has internal conversion logic, ensure that:
- The DataFrame index (from
self.original_dataandself.aggregated_data) is properly named (e.g., 'time') for correct xarray dimension naming. - The explicit conversion produces the expected Dataset structure that
with_plotlyrequires, or clarify whether the internal conversion inwith_plotlywould handle DataFrames differently.
Run the following script to verify the DataFrame structure and resulting xarray conversion:
Also applies to: 156-156
🏁 Script executed:
#!/bin/bash
# Description: Check the DataFrame creation and structure of original_data and aggregated_data
# Search for where original_data and aggregated_data are created/assigned
rg -n -B3 -A3 'self\.original_data\s*=' --type=py
# Search for where these DataFrames are populated or transformed
rg -n -B3 -A3 'self\.aggregated_data\s*=' --type=py
# Check if there are any index naming operations on these DataFrames
rg -n -B2 -A2 '\.index\.name\s*=' --type=py -g 'aggregation.py'Length of output: 1032
🏁 Script executed:
#!/bin/bash
# Check the __init__ method signature and type hints
rg -n -B2 -A15 'def __init__' flixopt/aggregation.py | head -40
# Check imports to understand tsam
rg -n 'import.*tsam|from.*tsam' flixopt/aggregation.py
# Check the plotting.with_plotly function signature and implementation
rg -n -B2 -A20 'def with_plotly' flixopt/plotting.py
# Check if DataFrame index is being set anywhere in the plot method
rg -n -B5 -A5 'def plot' flixopt/aggregation.py | head -60Length of output: 248
🏁 Script executed:
#!/bin/bash
# Check the __init__ method signature and type hints
rg -n -B2 -A15 'def __init__' flixopt/aggregation.py | head -50
# Check imports to understand tsam
rg -n 'import.*tsam|from.*tsam' flixopt/aggregation.py
# Check the plotting.with_plotly function signature and implementation
rg -n -B2 -A25 'def with_plotly' flixopt/plotting.py
# Check if DataFrame index is being set anywhere
rg -n '\.index\.name' flixopt/aggregation.pyLength of output: 3063
🏁 Script executed:
#!/bin/bash
# Get the full plot method implementation around lines 144-173
rg -n -B5 -A30 'def plot' flixopt/aggregation.py | grep -A 35 'def plot'
# Check with_plotly's internal conversion logic (_ensure_dataset)
rg -n -B2 -A15 '_ensure_dataset' flixopt/plotting.py
# Check if the plot method creates dataframes and how
rg -n 'df_org\|df_agg' flixopt/aggregation.py
# Check the Aggregation class to see data structure
ast-grep --pattern 'class Aggregation { $$$ }'Length of output: 5776
Remove redundant explicit .to_xarray() calls — pass DataFrames directly to with_plotly.
The explicit .to_xarray() conversions at lines 153 and 156 are unnecessary. The with_plotly function already handles DataFrame-to-Dataset conversion internally via _ensure_dataset() (line 473 in plotting.py), which calls .to_xarray() on DataFrames automatically. Passing DataFrames directly simplifies the code without changing behavior:
fig = plotting.with_plotly(df_org, 'line', colors=colormap, xlabel='Time in h')instead of:
fig = plotting.with_plotly(df_org.to_xarray(), 'line', colors=colormap, xlabel='Time in h')Apply this change to both lines 153 and 156.
🤖 Prompt for AI Agents
In flixopt/aggregation.py around lines 153 and 156, remove the redundant
`.to_xarray()` calls and pass the pandas DataFrame objects directly into
plotting.with_plotly (e.g., replace `plotting.with_plotly(df_org.to_xarray(),
...)` with `plotting.with_plotly(df_org, ...)` for both occurrences); ensure
function arguments and variable names remain unchanged and run tests/linters to
confirm no type issues.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/results.py (1)
1363-1372: Pass Datasets directly to dual_pie_with_matplotlib for consistency.Lines 1364–1365 convert
inputsandoutputsto pandas via.to_pandas()before passing todual_pie_with_matplotlib(). However, as of the changes inplotting.py(line 1263),dual_pie_with_matplotlib()now acceptsxr.Dataset | pd.DataFramedirectly and handles conversion internally viapreprocess_dataset_for_pie().Pass the Datasets directly for consistency with the Plotly path (lines 1350–1351) and to leverage the unified preprocessing:
elif engine == 'matplotlib': logger.debug('Parameter text_info is not supported for matplotlib') figure_like = plotting.dual_pie_with_matplotlib( - data_left=inputs.to_pandas(), - data_right=outputs.to_pandas(), + data_left=inputs, + data_right=outputs, colors=colors, title=title, subtitles=('Inputs', 'Outputs'), legend_title='Flows', lower_percentage_group=lower_percentage_group, **plot_kwargs, )
♻️ Duplicate comments (4)
flixopt/plotting.py (3)
394-687: ** px_kwargs still not forwarded to Plotly Express functions.**The parameter
**px_kwargsis accepted at line 408 but never merged intocommon_argsbefore callingpx.bar(),px.line(), orpx.area()(lines 621–632). This breaks the advertised kwargs passthrough capability documented in the docstring. Users expecting to override Plotly Express parameters will see no effect.Apply this fix to merge px_kwargs into common_args before creating the figure:
common_args = { 'data_frame': df_long, 'x': x_dim, 'y': 'value', 'color': 'variable', 'facet_row': facet_row, 'facet_col': facet_col, 'animation_frame': animate_by, 'color_discrete_map': color_discrete_map, 'title': title, 'labels': {'value': ylabel, x_dim: xlabel, 'variable': ''}, } # Add facet_col_wrap for single facet dimension if facet_col and not facet_row: common_args['facet_col_wrap'] = facet_cols + + # Merge user-provided px_kwargs (allows overriding defaults) + if px_kwargs: + common_args.update(px_kwargs) if mode == 'stacked_bar':
478-478: ** Remove stray quote from error message.**The leading double-quote leaks into the log output.
- logger.error('"with_plotly() got an empty Dataset.') + logger.error('with_plotly() got an empty Dataset.')
782-823: ** MultiIndex from to_dataframe() breaks bar width calculation.**
data.to_dataframe()(line 782) can return a DataFrame with a MultiIndex when the Dataset has multiple dimensions. Line 790 computes bar width viadf.index.to_series().diff().dropna().min(), which will fail or produce incorrect widths for MultiIndex or categorical indexes.Refactor to ensure a 1D numeric/datetime index or handle the MultiIndex case explicitly:
# Convert Dataset to DataFrame for matplotlib plotting (naturally wide-form) df = data.to_dataframe() + + # Handle MultiIndex: flatten to single time dimension if possible + if isinstance(df.index, pd.MultiIndex): + if 'time' in df.index.names: + df = df.reset_index().set_index('time').sort_index() + else: + # Use first level as index + df = df.reset_index().set_index(df.index.names[0]).sort_index() # Get colors in column order processed_colors = [color_discrete_map.get(str(col), '#808080') for col in df.columns] if mode == 'stacked_bar': cumulative_positive = np.zeros(len(df)) cumulative_negative = np.zeros(len(df)) - width = df.index.to_series().diff().dropna().min() # Minimum time difference + # Robust bar width calculation + try: + width = df.index.to_series().diff().dropna().min() + if pd.isna(width): + width = 0.8 + except Exception: + width = 0.8 # fallback for categorical/irregular indexAlternatively, raise a clear ValueError if the index is not 1D numeric/datetime to guide users.
flixopt/results.py (1)
110-123: ** Remove non-existent setup_colors() API from docstring.**The docstring documents a
setup_colors()method that does not exist in theCalculationResultsclass. Users following this example will encounter anAttributeErrorat runtime. Either implement this method or replace the example with working code.Replace with a working example:
Configure automatic color management for plots: ```python - # Dict-based configuration: - results.setup_colors({'Solar*': 'Oranges', 'Wind*': 'Blues', 'Battery': 'green'}) - - # All plots automatically use configured colors (colors=None is the default) - results['ElectricityBus'].plot_node_balance() - results['Battery'].plot_charge_state() - - # Override when needed - results['ElectricityBus'].plot_node_balance(colors='turbo') # Ignores setup + # Pass colors directly to individual plot calls + colors = {'Solar*': 'Oranges', 'Wind*': 'Blues', 'Battery': 'green'} + results['ElectricityBus'].plot_node_balance(colors=colors) + + # Or use a named colormap + results['Battery'].plot_charge_state(colors='turbo') ```
🧹 Nitpick comments (2)
flixopt/plotting.py (1)
1124-1124: Use logger.warning() instead of print() for consistency.Line 1124 uses
print()for a warning message. For consistency with the rest of the codebase and proper logging configuration, uselogger.warning()instead.- print(f'Warning: Negative value for {var}: {total_value}. Using absolute value.') + logger.warning(f'Negative value for {var}: {total_value}. Using absolute value.')flixopt/results.py (1)
1619-1626: Hardcoded charge_state line color overrides user customization.Line 1624 sets
trace.line.color = 'black'for the charge_state trace, which overrides any color customization users might pass viatrace_kwargsinplot_kwargs. While a consistent charge_state color is reasonable, consider making this configurable or documenting the behavior.Options:
- Allow users to override via a special key in
plot_kwargs(e.g.,charge_state_color='black')- Document in the docstring that charge_state is always plotted in black
- Respect trace color customization from
trace_kwargsif providedAdditionally, note that
plot_kwargsis passed to bothwith_plotly()calls (lines 1600 and 1616). If users providelayout_kwargs, they will be applied twice. Consider splitting kwargs or documenting this behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
flixopt/plotting.py(21 hunks)flixopt/results.py(34 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
flixopt/results.py (1)
flixopt/plotting.py (1)
with_matplotlib(690-837)
flixopt/plotting.py (2)
flixopt/config.py (1)
CONFIG(61-322)flixopt/aggregation.py (1)
plot(144-173)
Description
Improve plotting methods regarding Data validation and passing kwargs to underlying plotting methods
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests