Feature/plotting colors simplified #437
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
WalkthroughThis PR centralizes plotting configuration and color management: adds CONFIG.Plotting, a ColorProcessor and ElementColorResolver, CalculationResults.setup_colors(), xarray Dataset support in plotting functions, extended plotting kwargs, and updates examples, aggregation, and tests to use the new color API and defaults. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Results as CalculationResults
participant Resolver as ElementColorResolver
participant Processor as ColorProcessor
participant Plotting as Plotting Backend
User->>Results: setup_colors(mapping)
activate Results
Results->>Resolver: resolve(mapping, elements)
activate Resolver
Resolver->>Resolver: expand patterns -> variables
Resolver-->>Results: element->color map
deactivate Resolver
Results->>Results: store self.colors
deactivate Results
User->>Results: plot_* (data, colors=None)
activate Results
Results->>Processor: process_colors(self.colors, data labels)
activate Processor
Processor-->>Results: resolved colors per variable
deactivate Processor
Results->>Plotting: call with_plotly/with_matplotlib(data, colors=resolved)
Plotting-->>Results: Figure/Axes
deactivate Results
Results-->>User: Figure/Axes
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
flixopt/plotting.py (1)
2394-2407: Robust saving: ensure parent dirs exist and simplify Plotly type checkCreate output directories before saving; use go.Figure for isinstance check to avoid relying on top-level plotly import chain.
filename = user_path or default_path filename = filename.with_name(filename.name.replace('|', '__')) if filename.suffix == '': if default_filetype is None: raise ValueError('No default filetype provided') filename = filename.with_suffix(default_filetype) + # Ensure parent directory exists + filename.parent.mkdir(parents=True, exist_ok=True) - if isinstance(figure_like, plotly.graph_objs.Figure): + if isinstance(figure_like, go.Figure): fig = figure_likeAlso applies to: 2408-2410, 2454-2456
CHANGELOG.md (1)
96-96: Typo: “acessing” → “accessing”.-**Summary**: Fixed a bug when acessing the `effects_per_component` dataset in results without periodic effects. +**Summary**: Fixed a bug when accessing the `effects_per_component` dataset in results without periodic effects.
🧹 Nitpick comments (14)
examples/03_Calculation_types/example_calculation_types.py (1)
205-205: Dataset-native plotting migration looks goodCalls now pass xr.Dataset directly, and sum('time') leverages scalar-DS handling in with_plotly. Optionally consider fx.plotting.export_figure for consistent CONFIG-driven sizing/saving instead of write_html chaining.
Also applies to: 212-212, 219-219, 226-226, 233-233
flixopt/plotting.py (4)
789-793: Minor log message nitLeading quote in logger.error('"...') is likely unintended.
- logger.error('"with_plotly() got an empty Dataset.') + logger.error('with_plotly() got an empty Dataset.')
975-978: Guard against fig.frames being NoneSome px figures may have frames = None. Iterate safely.
- for frame in fig.frames: + for frame in (fig.frames or []): all_traces.extend(frame.data)
1121-1138: Datetime bar width handling in matplotlibwidth derived from Timedelta may not render as intended on datetime x-axis. Convert to Matplotlib date units or omit width and let mpl choose.
- width = df.index.to_series().diff().dropna().min() # Minimum time difference + from matplotlib.dates import date2num + if len(df.index) > 1: + xnum = date2num(df.index.to_pydatetime()) + width = np.min(np.diff(xnum)) + else: + width = 0.8
2003-2013: Heatmap colors type vs behaviorcolors is typed as ColorType but only string colormap is honored; lists/dicts are ignored. Either:
- Narrow type to str | None in both heatmap functions, or
- Add validation and raise on unsupported types to prevent silent fallback.
Also applies to: 2186-2203
examples/04_Scenarios/scenario_example.py (1)
199-200: Good: initialize consistent colors up frontsetup_colors() before plotting ensures stable color mapping across charts. Consider showcasing a small dict or YAML example in the example to demonstrate configurability.
tests/conftest.py (1)
842-855: Test env: disable showing plots by defaultSetting CONFIG.Plotting.default_show=False and applying once per session prevents GUI popups. Consider also fixing default_figure_width/height/dpi here to stabilize any image-based comparisons.
examples/02_Complex/complex_example_results.py (1)
33-35: Avoid opening many plot windows in loops.Default
show=Truemay pop up a window per bus. Recommend disablingshowin loops or saving instead.-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) + bus.plot_node_balance(show=False)flixopt/aggregation.py (1)
145-169: Unify colors for “Original” vs “Aggregated” and keep provided axis label.
- X-axis label passed via
xlabel='Time in h'is later overridden to "Index".- “Original - X” and “Aggregated - X” get different colors; use the same color per base variable for readability.
-def plot(self, colormap: str | None = None, show: bool = True, save: pathlib.Path | None = None) -> go.Figure: +def plot(self, colormap: "plotting.ColorType" | None = None, show: bool = True, save: pathlib.Path | None = None) -> go.Figure: @@ - for trace in fig.data: - trace.update(dict(line=dict(dash='dash'))) + # Dashed "Original" traces and capture their colors by base variable + original_color_by_base: dict[str, str | None] = {} + for trace in fig.data: + trace.update(dict(line=dict(dash='dash'))) + name = trace.name or "" + base = name.split(" - ", 1)[1] if " - " in name else name + original_color_by_base[base] = getattr(trace.line, "color", None) @@ - fig2 = plotting.with_plotly( + fig2 = plotting.with_plotly( df_agg.to_xarray(), 'line', colors=colormap or CONFIG.Plotting.default_qualitative_colorscale, xlabel='Time in h', ) - for trace in fig2.data: - fig.add_trace(trace) + # Recolor "Aggregated" traces to match their "Original" counterparts + for trace in fig2.data: + name = trace.name or "" + base = name.split(" - ", 1)[1] if " - " in name else name + color = original_color_by_base.get(base) + if color: + trace.update(line=dict(color=color)) + fig.add_trace(trace) @@ - fig.update_layout( - title='Original vs Aggregated Data (original = ---)', xaxis_title='Index', yaxis_title='Value' - ) + # Keep labeling from with_plotly; only set title + fig.update_layout(title='Original vs Aggregated Data (original = ---)')tests/test_plotting_api.py (1)
35-45: Also assert that trace kwargs are applied.Strengthen the passthrough test by checking line width on traces.
fig = plotting.with_plotly( sample_dataset, mode='line', trace_kwargs={'line': {'width': 5}}, layout_kwargs={'width': 1200, 'height': 600}, ) assert fig.layout.width == 1200 assert fig.layout.height == 600 + assert all(getattr(t.line, "width", None) == 5 for t in fig.data)examples/01_Simple/simple_example.py (1)
48-51: Prefer descriptive Flow labels for better legends.Labels
'0'..'9'produce non-informative legends. Consider meaningful names to improve plot readability.-outputs=[fx.Flow(label=str(i), bus='Fernwärme', size=5) for i in range(10)], +outputs=[fx.Flow(label=f'Q_th_{i}', bus='Fernwärme', size=5) for i in range(10)],flixopt/config.py (1)
324-327: Plotly template application looks good. Consider documenting precedence.Minor: Clarify in docstring that explicit figure templates override
CONFIG.Plotting.plotly_templateif users set them per-plot.flixopt/results.py (2)
1466-1471: Build combined_ds with xr.merge to avoid silent key overwrites.
Dict unpack may drop variables if names collide; xr.merge preserves union safely.- # Combine inputs and outputs to resolve colors for all variables - combined_ds = xr.Dataset({**inputs.data_vars, **outputs.data_vars}) + # Combine inputs and outputs robustly (preserve all vars even if names collide) + combined_ds = xr.merge([inputs, outputs], compat='override', join='outer')
2169-2185: Type/doc mismatch for colors; align with module’s ColorType.
Heatmap path acceptsplotting.ColorType | Noneelsewhere; keep consistent here too.- colors: str | None = None, + colors: plotting.ColorType | None = None, @@ - colors: Color scheme (default: None uses CONFIG.Plotting.default_sequential_colorscale). - See plotting.ColorType for options. + colors: Color scheme (default: None uses CONFIG.Plotting.default_sequential_colorscale). + See plotting.ColorType for options.Also applies to: 2194-2196
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.md(1 hunks)examples/01_Simple/simple_example.py(2 hunks)examples/02_Complex/complex_example.py(1 hunks)examples/02_Complex/complex_example_results.py(1 hunks)examples/03_Calculation_types/example_calculation_types.py(1 hunks)examples/04_Scenarios/scenario_example.py(1 hunks)flixopt/aggregation.py(3 hunks)flixopt/config.py(7 hunks)flixopt/plotting.py(40 hunks)flixopt/results.py(46 hunks)tests/conftest.py(2 hunks)tests/test_plotting_api.py(1 hunks)tests/test_results_plots.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
tests/conftest.py (1)
flixopt/config.py (3)
CONFIG(75-422)Plotting(202-261)apply(286-327)
flixopt/aggregation.py (2)
flixopt/config.py (2)
CONFIG(75-422)Plotting(202-261)flixopt/plotting.py (1)
with_plotly(689-1009)
examples/01_Simple/simple_example.py (3)
flixopt/components.py (1)
Source(1115-1219)flixopt/elements.py (1)
Flow(234-494)flixopt/results.py (3)
size(1889-1897)setup_colors(331-427)setup_colors(2091-2149)
examples/02_Complex/complex_example.py (1)
flixopt/results.py (8)
setup_colors(331-427)setup_colors(2091-2149)plot_heatmap(824-966)plot_heatmap(2166-2272)plot_heatmap(2300-2523)plot_node_balance(1131-1327)plot_charge_state(1602-1812)plot_node_balance_pie(1329-1509)
examples/03_Calculation_types/example_calculation_types.py (1)
flixopt/plotting.py (1)
with_plotly(689-1009)
examples/04_Scenarios/scenario_example.py (1)
flixopt/results.py (2)
setup_colors(331-427)setup_colors(2091-2149)
tests/test_plotting_api.py (1)
flixopt/plotting.py (2)
with_plotly(689-1009)_ensure_dataset(626-634)
examples/02_Complex/complex_example_results.py (2)
flixopt/results.py (5)
setup_colors(331-427)setup_colors(2091-2149)plot_network(968-988)plot_node_balance(1131-1327)plot_node_balance_pie(1329-1509)flixopt/structure.py (1)
values(1134-1135)
flixopt/results.py (2)
flixopt/config.py (3)
CONFIG(75-422)reset(266-283)Plotting(202-261)flixopt/plotting.py (7)
ElementColorResolver(350-514)resolve(386-440)resolve_colors(668-686)with_matplotlib(1012-1170)dual_pie_with_plotly(1674-1811)dual_pie_with_matplotlib(1814-1998)with_plotly(689-1009)
flixopt/plotting.py (3)
flixopt/config.py (2)
CONFIG(75-422)Plotting(202-261)flixopt/aggregation.py (1)
plot(145-184)tests/test_results_plots.py (2)
show(10-11)save(20-21)
⏰ 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.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
🔇 Additional comments (11)
tests/test_results_plots.py (1)
31-31: Align tests with new default colormapSwitching to 'turbo' for string-based colors and heatmaps matches the new defaults and API. LGTM.
Also applies to: 54-55
examples/01_Simple/simple_example.py (1)
113-115: LGTM — example shows the intended color-setup flow.flixopt/results.py (9)
73-76: Good: clear public contract for auto-colors.
Concise, actionable doc; default/fallback behavior is explicit.
115-128: Docs/readme-style example adds usability.
Examples cover dict, override, and None behavior.
262-264: Sane default for colors state.
Explicit None avoids implicit behavior; aligns with docs.
828-845: **API surface additions (colors/show/facet_cols/plot_kwargs) look coherent.
Signature aligns with module-level plot_heatmap and CONFIG defaults.
1922-1934: Segment-level colors: clear docs on propagation.
Accurately explains propagation behavior.
2075-2077: Sane default for segmented colors.
Consistent with CalculationResults.
2304-2322: Heatmap path defaulting and dpi forwarding: solid.
Defaults from CONFIG applied only when colors is None; dpi plumbed via export.Also applies to: 2477-2483
1270-1273: ****The review comment incorrectly claims that
with_plotlydoes not acceptxlabel. The function signature at line 689 explicitly showsxlabel: str = ''as a supported parameter, and documentation at line 722 confirms this is "The label for the x-axis." Additionally, the function also acceptslayout_kwargsas a parameter (line 703), so both approaches are valid.The function uses
xlabelinternally to setxaxis_titleinupdate_layout(line 833), confirming that passingxlabel='Time in h'will not raise a TypeError. The original code is correct and does not require the suggested changes.Likely an incorrect or invalid review comment.
1407-1409: dpi and show forwarding verified—no issues found.The
export_figurefunction signature confirms:
show: bool | None = None— accepts None, which triggersCONFIG.Plotting.default_showdpi: int | None = None— accepts None, which triggersCONFIG.Plotting.default_dpiAll four call sites (lines 1319, 1501, 1804, 2515) consistently pass both parameters. The dpi extraction at line 1407 correctly uses None as the default.
| - `CONFIG.Plotting.default_sequential_colorscale` - Falls back to template's sequential colorscale when `None` | ||
| - `CONFIG.Plotting.default_qualitative_colorscale` - Falls back to template's colorway when `None` | ||
| - `CONFIG.Plotting.default_show` defaults to `True` - set to None to prevent unwanted GUI windows | ||
There was a problem hiding this comment.
Correct guidance for disabling default plot display.
CONFIG.Plotting.default_show is a bool. Use False (not None) to prevent GUI windows.
- - `CONFIG.Plotting.default_show` defaults to `True` - set to None to prevent unwanted GUI windows
+ - `CONFIG.Plotting.default_show` defaults to `True` — set to `False` to prevent unwanted GUI windows📝 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.
| - `CONFIG.Plotting.default_sequential_colorscale` - Falls back to template's sequential colorscale when `None` | |
| - `CONFIG.Plotting.default_qualitative_colorscale` - Falls back to template's colorway when `None` | |
| - `CONFIG.Plotting.default_show` defaults to `True` - set to None to prevent unwanted GUI windows | |
| - `CONFIG.Plotting.default_sequential_colorscale` - Falls back to template's sequential colorscale when `None` | |
| - `CONFIG.Plotting.default_qualitative_colorscale` - Falls back to template's colorway when `None` | |
| - `CONFIG.Plotting.default_show` defaults to `True` — set to `False` to prevent unwanted GUI windows |
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 63 to 66, the guidance about disabling default plot
display is incorrect: CONFIG.Plotting.default_show is a boolean and you must set
it to False (not None) to prevent GUI windows; update the line that reads "set
to None to prevent unwanted GUI windows" to "set to False to prevent unwanted
GUI windows" and ensure the entry clearly states the default is True and False
disables display.
| colors = self.processor._generate_colors_from_colormap( | ||
| self.processor.default_colorscale, len(element_names) | ||
| ) | ||
| return dict(zip(element_names, colors, strict=False)) | ||
|
|
||
| # Process config entries |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid zip(strict=...) for wider Python compatibility
zip(strict=...) requires Python ≥ 3.10. If you support 3.9, this will crash. Use plain zip (inputs are same length) or add a length check.
Apply this diff:
- return dict(zip(element_names, colors, strict=False))
+ return dict(zip(element_names, colors))
...
- element_colors.update(zip(matched, colors, strict=False))
+ element_colors.update(zip(matched, colors))
...
- element_colors.update(zip(value, colors, strict=False))
+ element_colors.update(zip(value, colors))
...
- element_colors.update(zip(missing, colors, strict=False))
+ element_colors.update(zip(missing, colors))Run to find any remaining occurrences:
Also applies to: 473-489, 491-496
🏁 Script executed:
#!/bin/bash
rg -nP '\bzip\s*\([^)]*strict\s*=' -C2Length of output: 2777
Based on the script results and my verification, I've found a critical issue with the review comment: it is incomplete. The review identifies only 4 occurrences of zip(..., strict=False) in flixopt/plotting.py, but there are actually 2 additional occurrences that were missed:
-
The strict parameter was added to zip() in Python 3.10 and is not available in 3.9 or earlier — the review's Python version requirement claim is accurate.
-
Missing occurrences not mentioned in the review:
flixopt/plotting.py:1098- anotherzip(..., strict=False)callflixopt/results.py:798- anotherzip(..., strict=False)call
Both of these should be fixed alongside the ones already identified in the review to ensure comprehensive compatibility.
Remove zip(strict=False) and use plain zip() for Python 3.9 compatibility
The 4 occurrences in plotting.py lines 464, 476, 488, 494 should be fixed as suggested. Additionally, fix the 2 missing occurrences:
flixopt/plotting.py:1098- line withzip(variables, values, strict=False)flixopt/results.py:798- line withzip(components_list, dataarrays, strict=False)
Change all from zip(..., strict=False) to plain zip(...) since the inputs are known to be equal length in each context.
🤖 Prompt for AI Agents
In flixopt/plotting.py around lines 461-466 (and also update the other
occurrences at 464, 476, 488, 494, and 1098) and flixopt/results.py at line 798,
the code uses zip(..., strict=False) which is unsupported on Python 3.9; remove
the strict=False keyword from each zip call and use plain zip(...) instead (the
inputs are guaranteed equal length here), ensuring all six occurrences are
changed so the code remains Python 3.9 compatible.
| if colors is None: | ||
| colors = CONFIG.Plotting.default_qualitative_colorscale | ||
|
|
There was a problem hiding this comment.
Unused parameter: lower_percentage_group
lower_percentage_group is in the signature/docs but not applied. Either implement grouping of small slices into "Other" or remove the parameter to avoid API confusion.
Also applies to: 1682-1685
| def setup_colors( | ||
| self, | ||
| config: dict[str, str | list[str]] | str | pathlib.Path | None = None, | ||
| *, | ||
| default_colorscale: str | None = None, | ||
| reset: bool = True, | ||
| ) -> dict[str, str]: | ||
| """Configure colors for plotting. Returns variable→color dict. | ||
| Supports multiple configuration styles: | ||
| - Direct assignment: {'Boiler1': 'red'} | ||
| - Pattern matching: {'Solar*': 'orange'} or {'Solar*': 'Oranges'} | ||
| - Family grouping: {'oranges': ['Solar1', 'Solar2']} | ||
| Args: | ||
| config: Optional color configuration: | ||
| - dict: Component/pattern to color/colorscale mapping | ||
| - str/Path: Path to YAML file | ||
| - None: Use default colorscale for all components | ||
| default_colorscale: Default colorscale for unmapped components. | ||
| Defaults to CONFIG.Plotting.default_qualitative_colorscale | ||
| reset: If True, reset all existing colors before applying config. | ||
| If False, only update/add specified components (default: True) | ||
| Returns: | ||
| dict[str, str]: Complete variable→color mapping | ||
| Examples: | ||
| Direct color assignment: | ||
| ```python | ||
| results.setup_colors({'Boiler1': 'red', 'CHP': 'darkred'}) | ||
| ``` | ||
| Pattern matching with color: | ||
| ```python | ||
| results.setup_colors({'Solar*': 'orange', 'Wind*': 'blue'}) | ||
| ``` | ||
| Pattern matching with colorscale (generates shades): | ||
| ```python | ||
| results.setup_colors({'Solar*': 'Oranges', 'Wind*': 'Blues'}) | ||
| ``` | ||
| Family grouping (colorscale samples): | ||
| ```python | ||
| results.setup_colors( | ||
| { | ||
| 'oranges': ['Solar1', 'Solar2'], | ||
| 'blues': ['Wind1', 'Wind2'], | ||
| } | ||
| ) | ||
| ``` | ||
| Load from YAML file: | ||
| ```python | ||
| # colors.yaml: | ||
| # Boiler1: red | ||
| # Solar*: Oranges | ||
| # oranges: | ||
| # - Solar1 | ||
| # - Solar2 | ||
| results.setup_colors('colors.yaml') | ||
| ``` | ||
| Merge with existing colors: | ||
| ```python | ||
| results.setup_colors({'Boiler1': 'red'}) | ||
| results.setup_colors({'CHP': 'blue'}, reset=False) # Keeps Boiler1 red | ||
| ``` | ||
| Disable automatic coloring: | ||
| ```python | ||
| results.colors = None # Plots use default colorscales | ||
| ``` | ||
| """ | ||
| # Create resolver and delegate | ||
| resolver = plotting.ElementColorResolver( | ||
| self.components, | ||
| default_colorscale=default_colorscale, | ||
| engine='plotly', | ||
| ) | ||
|
|
||
| # Resolve colors (with variable-level merging if reset=False) | ||
| self.colors = resolver.resolve( | ||
| config=config, | ||
| reset=reset, | ||
| existing_colors=None if reset else self.colors, | ||
| ) | ||
|
|
||
| return self.colors | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
ElementColorResolver attribute mismatch risk (_variables vs _variable_names).
Resolver summary mentions using an element’s _variables; here elements are ComponentResults which expose _variable_names. If Resolver expects _variables, color expansion may fail at runtime.
Proposed compatibility shim (minimal surface change):
# Add to class _ElementResults (near other @property getters)
@property
def _variables(self) -> list[str]:
return self._variable_namesRun to verify expectations and usages:
🏁 Script executed:
#!/bin/bash
# Check Resolver implementation and expected attribute on elements
rg -nP 'class\s+ElementColorResolver\b|_expand_to_variables|_variables\b|_variable_names\b' -C3 flixopt/plotting.py
# Confirm ComponentResults/_ElementResults expose only _variable_names today
rg -nP '\bclass\s+_ElementResults\b|\b_variable_names\b|\b_variables\b' -C2 flixopt/results.py
# Check for any reliance on `._variables` elsewhere
rg -nP '\._variables\b' -nLength of output: 3866
I need to verify that ComponentResults is actually passed to ElementColorResolver and confirm there are no other places in the codebase expecting ._variables on these result classes.Web search wasn't helpful for this codebase-specific question. Let me run shell scripts to verify the exact connection between setup_colors and what gets passed to ElementColorResolver.
🏁 Script executed:
#!/bin/bash
# Check what is passed to ElementColorResolver in setup_colors
rg -nP 'def setup_colors|ElementColorResolver\(' -C5 flixopt/results.py | head -50
# Also check the resolve method signature and how it accesses elements
rg -nP 'def resolve\(|self\.elements' -C3 flixopt/plotting.py | head -80
# Find where setup_colors is called and what context is available
rg -nP 'setup_colors\(' -C2 flixopt/results.py | head -30Length of output: 3525
🏁 Script executed:
#!/bin/bash
# Check ComponentResults class definition and inheritance
rg -nP 'class ComponentResults|class BusResults|class FlowResults' -C3 flixopt/results.py | head -40
# Check self.components initialization in CalculationResults or similar
rg -nP 'self\.components\s*=' -C2 flixopt/results.py | head -30
# Confirm ComponentResults extends _ElementResults
rg -nP 'class ComponentResults\(|class BusResults\(|class FlowResults\(' flixopt/results.pyLength of output: 1014
🏁 Script executed:
#!/bin/bash
# Check _NodeResults class definition
rg -nP 'class _NodeResults' -C5 flixopt/results.py
# Check the full inheritance hierarchy for ComponentResults
rg -nP 'class _NodeResults\(|class _ElementResults\(' flixopt/results.pyLength of output: 342
ElementColorResolver tries to access ._variables on element results, but only ._variable_names is available—fix will cause runtime AttributeError.
The mismatch is confirmed:
- Line 512 in
flixopt/plotting.pyaccessesself.elements[element_name]._variables self.components(passed asself.elements) containsComponentResultsinstancesComponentResults→_NodeResults→_ElementResults, which defines onlyself._variable_names(line 1046 inflixopt/results.py), not_variables- This causes
AttributeErrorwhensetup_colors()callsresolve()and reaches_expand_to_variables()
Apply the proposed compatibility shim:
# Add to class _ElementResults (near other @property getters)
@property
def _variables(self) -> list[str]:
return self._variable_names🤖 Prompt for AI Agents
flixopt/results.py around lines 1040–1060: _ElementResults exposes
_variable_names but code elsewhere expects a _variables attribute, causing
AttributeError when setup_colors() calls ElementColorResolver; add a
compatibility property to _ElementResults named _variables that simply returns
self._variable_names (as a read-only list) so existing callers continue to work
without changing other code.
| # Extract dpi for export_figure | ||
| dpi = plot_kwargs.pop('dpi', None) # None uses CONFIG.Plotting.default_dpi | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
with_plotly axis label: same issue; use layout_kwargs instead of xlabel.
Mirror the fix from plot_node_balance to avoid unexpected kwarg errors.
- # Extract dpi for export_figure
- dpi = plot_kwargs.pop('dpi', None) # None uses CONFIG.Plotting.default_dpi
+ # Extract dpi and layout kwargs
+ dpi = plot_kwargs.pop('dpi', None) # None uses CONFIG.Plotting.default_dpi
+ layout_kwargs = plot_kwargs.pop('layout_kwargs', {}) or {}
+ layout_kwargs.setdefault('xaxis_title', 'Time in h')
if engine == 'plotly':
# Plot flows (node balance) with the specified mode
figure_like = plotting.with_plotly(
ds,
facet_by=facet_by,
animate_by=animate_by,
- colors=resolved_colors,
+ colors=resolved_colors,
mode=mode,
title=title,
facet_cols=facet_cols,
- xlabel='Time in h',
- **plot_kwargs,
+ layout_kwargs=layout_kwargs,
+ **plot_kwargs,
)
@@
charge_state_fig = plotting.with_plotly(
charge_state_ds,
facet_by=facet_by,
animate_by=animate_by,
- colors=resolved_colors,
+ colors=resolved_colors,
mode='line', # Always line for charge_state
title='', # No title needed for this temp figure
facet_cols=facet_cols,
- xlabel='Time in h',
- **plot_kwargs,
+ layout_kwargs=layout_kwargs,
+ **plot_kwargs,
)Same script as for plot_node_balance (checks for layout_kwargs vs xlabel).
Also applies to: 1723-1734, 1741-1750
---
🏁 Script executed:
```shell
#!/bin/bash
# Check current code at lines 1697-1699
echo "=== Lines 1697-1699 ==="
sed -n '1697,1699p' flixopt/results.py
echo -e "\n=== Lines 1723-1734 ==="
sed -n '1723,1734p' flixopt/results.py
echo -e "\n=== Lines 1741-1750 ==="
sed -n '1741,1750p' flixopt/results.py
Length of output: 1014
🏁 Script executed:
#!/bin/bash
# Find with_plotly function definition
echo "=== Searching for with_plotly function ==="
rg -nP '\bdef with_plotly' --type py
echo -e "\n=== Searching for plot_node_balance function ==="
rg -nP '\bdef plot_node_balance' --type pyLength of output: 239
🏁 Script executed:
#!/bin/bash
# Check context around plot_node_balance to see the fix pattern
echo "=== Searching for xlabel usage in plot_node_balance context ==="
rg -nP -C 10 'plot_node_balance' flixopt/results.py | head -100Length of output: 4096
🏁 Script executed:
#!/bin/bash
# Find all with_plotly calls with xlabel parameter to understand current patterns
echo "=== All with_plotly calls with xlabel ==="
rg -nP -B 3 "xlabel\s*=\s*['\"]" flixopt/results.py | head -80Length of output: 571
---
🏁 Script executed:
```shell
#!/bin/bash
# Check with_plotly function signature and parameters
echo "=== with_plotly function signature ==="
sed -n '689,750p' flixopt/plotting.py
Length of output: 3076
🏁 Script executed:
#!/bin/bash
# Check plot_node_balance_pie function and see what fix exists at line 1305
echo "=== Context around line 1305 in plot_node_balance_pie ==="
sed -n '1290,1320p' flixopt/results.pyLength of output: 1047
🏁 Script executed:
#!/bin/bash
# Search for layout_kwargs usage pattern in results.py
echo "=== Existing layout_kwargs usage in results.py ==="
rg -nP -B 2 -A 2 'layout_kwargs' flixopt/results.pyLength of output: 1199
---
🏁 Script executed:
```shell
#!/bin/bash
# Find plot_node_balance implementation to see if it already uses layout_kwargs pattern
echo "=== plot_node_balance implementation (lines 1131-1200) ==="
sed -n '1131,1250p' flixopt/results.py | grep -A 100 'def plot_node_balance'
Length of output: 5469
🏁 Script executed:
#!/bin/bash
# Search for where layout_kwargs is actually extracted/created in plot_node_balance
echo "=== Search for layout_kwargs extraction in plot_node_balance context ==="
rg -nP -B 5 -A 15 "layout_kwargs.*=.*plot_kwargs.pop" flixopt/results.pyLength of output: 72
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the actual with_plotly calls in plot_node_balance function
echo "=== plot_node_balance with_plotly calls ==="
sed -n '1250,1320p' flixopt/results.py
Length of output: 2992
🏁 Script executed:
#!/bin/bash
# Find all with_plotly calls to understand the pattern across the file
echo "=== All with_plotly calls in results.py ==="
rg -nP -B 2 -A 10 'plotting\.with_plotly\(' flixopt/results.pyLength of output: 1606
Extract and pass layout_kwargs instead of xlabel to with_plotly calls.
The with_plotly function supports layout_kwargs as the proper way to customize axis labels via fig.update_layout(). Replace direct xlabel parameters with layout_kwargs containing xaxis_title.
Extract layout_kwargs from plot_kwargs at lines 1697-1699, then apply the fix to the two with_plotly calls at lines 1723-1734 and 1741-1750:
- # Extract dpi for export_figure
- dpi = plot_kwargs.pop('dpi', None) # None uses CONFIG.Plotting.default_dpi
+ # Extract dpi and layout kwargs
+ dpi = plot_kwargs.pop('dpi', None) # None uses CONFIG.Plotting.default_dpi
+ layout_kwargs = plot_kwargs.pop('layout_kwargs', {}) or {}
+ layout_kwargs.setdefault('xaxis_title', 'Time in h')
if engine == 'plotly':
# Plot flows (node balance) with the specified mode
figure_like = plotting.with_plotly(
ds,
facet_by=facet_by,
animate_by=animate_by,
- colors=resolved_colors,
+ colors=resolved_colors,
mode=mode,
title=title,
facet_cols=facet_cols,
- xlabel='Time in h',
- **plot_kwargs,
+ layout_kwargs=layout_kwargs,
+ **plot_kwargs,
)
@@
charge_state_fig = plotting.with_plotly(
charge_state_ds,
facet_by=facet_by,
animate_by=animate_by,
- colors=resolved_colors,
+ colors=resolved_colors,
mode='line', # Always line for charge_state
title='', # No title needed for this temp figure
facet_cols=facet_cols,
- xlabel='Time in h',
- **plot_kwargs,
+ layout_kwargs=layout_kwargs,
+ **plot_kwargs,
)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In flixopt/results.py around lines 1697-1699, plot_kwargs currently pops only
'dpi'; also pop 'layout_kwargs' from plot_kwargs (e.g. layout_kwargs =
plot_kwargs.pop('layout_kwargs', {})) and if an 'xlabel' exists in plot_kwargs,
move it into layout_kwargs as layout_kwargs.setdefault('xaxis_title',
plot_kwargs.pop('xlabel')) so axis labels are supplied via layout_kwargs; then
update the two with_plotly calls at lines ~1723-1734 and ~1741-1750 to remove
passing xlabel and instead pass the constructed layout_kwargs into with_plotly.
| # Setup colors on first segment | ||
| self.segment_results[0].setup_colors(config, default_colorscale=default_colorscale, reset=reset) | ||
|
|
||
| # Propagate to all other segments | ||
| for segment in self.segment_results[1:]: | ||
| segment.colors = self.segment_results[0].colors | ||
|
|
||
| # Store reference | ||
| self.colors = self.segment_results[0].colors | ||
|
|
||
| return self.colors |
There was a problem hiding this comment.
Guard against empty segments and consider union-based color resolution.
- Empty segment_results will crash at Line 2140. Add a clear error.
- Seeding from the first segment misses vars unique to later segments, yielding inconsistent colors.
- # Setup colors on first segment
- self.segment_results[0].setup_colors(config, default_colorscale=default_colorscale, reset=reset)
+ # Setup colors on first segment
+ if not self.segment_results:
+ raise ValueError('setup_colors: no segment_results available')
+ self.segment_results[0].setup_colors(config, default_colorscale=default_colorscale, reset=reset)Optional union-based approach (for later):
# Collect all elements across segments for a complete mapping
elements = {}
for seg in self.segment_results:
elements.update(seg.components)
resolver = plotting.ElementColorResolver(elements, default_colorscale=default_colorscale, engine='plotly')
colors_map = resolver.resolve(config=config, reset=reset, existing_colors=None if reset else self.colors)
for seg in self.segment_results:
seg.colors = colors_map
self.colors = colors_map🤖 Prompt for AI Agents
In flixopt/results.py around lines 2139 to 2149, guard against an empty
self.segment_results by raising a clear ValueError if no segments exist to
prevent the crash at line 2140, and replace seeding colors from only the first
segment with a union-based resolution: collect components across all segments,
call the plotting.ElementColorResolver once with that combined elements mapping
(using default_colorscale, config, reset, and existing self.colors when reset is
False) to produce a single colors_map, then assign that colors_map to each
segment's .colors and to self.colors so variables unique to later segments
receive consistent colors across all segments.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
examples/02_Complex/complex_example.py (1)
209-210: LGTM: wildcard fixes prior label mismatch.'BHKW*' covers 'BHKW2'; direct color names OK.
flixopt/config.py (1)
408-419: Resolved: to_dict now serializes only defined Plotting attrs.Removes prior AttributeError risk.
flixopt/plotting.py (2)
464-495: Remove zip(strict=...) for Python 3.9 compatibility.strict parameter is 3.10+; inputs are aligned, so plain zip() suffices.
- return dict(zip(element_names, colors, strict=False)) + return dict(zip(element_names, colors)) @@ - element_colors.update(zip(matched, colors, strict=False)) + element_colors.update(zip(matched, colors)) @@ - element_colors.update(zip(value, colors, strict=False)) + element_colors.update(zip(value, colors)) @@ - element_colors.update(zip(missing, colors, strict=False)) + element_colors.update(zip(missing, colors))
1096-1102: Remove zip(strict=...) here as well.Same Py3.9 issue in matplotlib scalar plot path.
- for i, (var, val) in enumerate(zip(variables, values, strict=False)): + for i, (var, val) in enumerate(zip(variables, values)):
🧹 Nitpick comments (2)
flixopt/plotting.py (2)
1710-1811: Implement lower_percentage_group for Plotly dual pies (parity with Matplotlib).Currently unused; group tiny slices into “Other” when threshold > 0.
- # Get data for left and right - left_labels, left_values = dataset_to_pie_data(data_left) - right_labels, right_values = dataset_to_pie_data(data_right) + # Get data for left and right + left_labels, left_values = dataset_to_pie_data(data_left) + right_labels, right_values = dataset_to_pie_data(data_right) + + # Optional grouping of smallest parts into "Other" + def group_small(labels, values, threshold_pct): + if not threshold_pct or not values: + return labels, values + s = pd.Series(values, index=labels).sort_values() + total = s.sum() + if total <= 0: + return labels, values + cum_pct = (s.cumsum() / total) * 100 + mask = cum_pct <= float(threshold_pct) + if mask.sum() > 1: + other_sum = s[mask].sum() + s = s[~mask] + if other_sum > 0: + s.loc['Other'] = other_sum + return list(s.index), list(s.values) + + left_labels, left_values = group_small(left_labels, left_values, lower_percentage_group) + right_labels, right_values = group_small(right_labels, right_values, lower_percentage_group)
789-793: Nit: fix stray quote in error message.Cosmetic only.
- logger.error('"with_plotly() got an empty Dataset.') + logger.error('with_plotly() got an empty Dataset.')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/01_Simple/simple_example.py(1 hunks)examples/02_Complex/complex_example.py(1 hunks)examples/02_Complex/complex_example_results.py(1 hunks)flixopt/config.py(7 hunks)flixopt/plotting.py(40 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
examples/02_Complex/complex_example.py (1)
flixopt/results.py (2)
setup_colors(331-427)setup_colors(2091-2149)
examples/01_Simple/simple_example.py (1)
flixopt/results.py (2)
setup_colors(331-427)setup_colors(2091-2149)
examples/02_Complex/complex_example_results.py (2)
flixopt/results.py (5)
setup_colors(331-427)setup_colors(2091-2149)plot_network(968-988)plot_node_balance(1131-1327)plot_node_balance_pie(1329-1509)flixopt/plotting.py (1)
plot_network(1344-1425)
flixopt/plotting.py (3)
flixopt/config.py (3)
CONFIG(75-420)Plotting(202-261)reset(266-283)flixopt/aggregation.py (1)
plot(145-184)tests/test_results_plots.py (2)
show(10-11)save(20-21)
⏰ 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 (8)
examples/01_Simple/simple_example.py (1)
115-116: LGTM: correct use of setup_colors.Keys match component labels; good placement post-solve.
examples/02_Complex/complex_example_results.py (1)
31-33: LGTM: bus-wise plotting sequence is clear.Assuming results.buses is dict-like in this context.
flixopt/config.py (6)
57-70: LGTM: plotting defaults added to _DEFAULTS.Provides single source of truth for reset/apply paths.
202-262: LGTM: CONFIG.Plotting API is well-scoped and typed.Docstrings and literals are clear.
279-281: LGTM: reset covers Plotting section.Keeps state consistent after CONFIG.reset().
324-328: LGTM: apply() sets Plotly template safely.No-op if plotly missing.
368-371: LGTM: YAML application supports plotting dict.Consistent with logging/modeling handling.
689-724: LGTM: _apply_plotting_config handles template set robustly.Error-tolerant import; fine default behavior.
| # --- Configure Color Mapping for Consistent Plot Colors (Optional) --- | ||
| results.setup_colors({'Solar*': 'oranges', 'Wind*': 'blues'}) # Dict (simplest) | ||
|
|
There was a problem hiding this comment.
Use canonical colorscale names to avoid fallback.
Plotly sequential scales are case-sensitive; prefer 'Oranges', 'Blues'.
-results.setup_colors({'Solar*': 'oranges', 'Wind*': 'blues'}) # Dict (simplest)
+results.setup_colors({'Solar*': 'Oranges', 'Wind*': 'Blues'}) # Dict (simplest)🤖 Prompt for AI Agents
In examples/02_Complex/complex_example_results.py around lines 21 to 23, the
colorscale names used in results.setup_colors({'Solar*': 'oranges', 'Wind*':
'blues'}) are lowercase and Plotly is case-sensitive; replace them with
canonical capitalized sequential scale names such as 'Oranges' and 'Blues' so
the provided scales are recognized and no fallback occurs.
| def _get_sequential_colorscale(self, colormap_name: str, num_colors: int) -> list[str] | None: | ||
| try: | ||
| colorscale = px.colors.get_colorscale(colormap_name) | ||
| # Generate evenly spaced points | ||
| color_points = [i / (num_colors - 1) for i in range(num_colors)] if num_colors > 1 else [0] | ||
| return px.colors.sample_colorscale(colorscale, color_points) | ||
| except PlotlyError: | ||
| return None | ||
|
|
There was a problem hiding this comment.
Handle case-insensitive sequential colorscales.
Without normalization, 'oranges' may fallback to default. Try title() as fallback.
def _get_sequential_colorscale(self, colormap_name: str, num_colors: int) -> list[str] | None:
try:
- colorscale = px.colors.get_colorscale(colormap_name)
+ colorscale = px.colors.get_colorscale(colormap_name)
except PlotlyError:
- return None
+ # Retry with canonical capitalization (e.g., 'oranges' -> 'Oranges')
+ try:
+ colorscale = px.colors.get_colorscale(colormap_name.title())
+ except PlotlyError:
+ return None
# Generate evenly spaced points
color_points = [i / (num_colors - 1) for i in range(num_colors)] if num_colors > 1 else [0]
return px.colors.sample_colorscale(colorscale, color_points)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In flixopt/plotting.py around lines 167 to 175, the sequential colorscale lookup
is case-sensitive so names like "oranges" can fall back to default; update the
method to try px.colors.get_colorscale(colormap_name) and if it raises
PlotlyError (or returns None) retry with colormap_name.title() (and optionally
colormap_name.lower() before title() if needed), then generate color_points and
call px.colors.sample_colorscale as before; if both attempts fail return None.
Ensure the function still returns list[str] | None and only catch PlotlyError
for the lookup attempts.
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Breaking Changes
New Features
Bug Fixes
Tests