Uh oh!
There was an error while loading. Please reload this page.
Feature/many simple improvements - #342
Conversation
5502afe to
06528a9CompareWalkthroughThe update revises plotting validations, color mapping, and heatmap rendering in flixopt/plotting.py, including stricter error handling and export behavior tweaks. It also adjusts docstrings and a log message in flixopt/results.py without changing functionality or public signatures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Caller
participant P as plotting.heat_map_data_from_df
participant M as Matplotlib/Plotly Heatmap
U->>P: provide DataFrame + format params
Note over P: Validate: non-empty DF, supported format/mode
alt invalid input
P-->>U: raise ValueError
else valid
P-->>U: return Z, x, y, colors
U->>M: render heatmap with shading='auto' (Matplotlib) / normal flow (Plotly)
M-->>U: figure/axes
end
sequenceDiagram
autonumber
participant U as Caller
participant E as plotting.export_figure
participant FS as Filesystem/Viewer
U->>E: figure_like, path
alt Plotly figure with non-.html suffix
E->>E: log warning, adjust suffix to .html
E->>FS: write HTML
else tuple/backends
E->>FS: save/show (plt.show() in some branches)
end
E-->>U: completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
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 (2)
flixopt/plotting.py (2)
756-759: Don’t useassertfor user input validation.
assertcan be optimized away; use an explicit exception to keep behavior consistent in production.Apply this change:
- assert pd.api.types.is_datetime64_any_dtype(df.index), (- 'The index of the DataFrame must be datetime to transform it properly for a heatmap plot'- )+ if not pd.api.types.is_datetime64_any_dtype(df.index):+ raise TypeError('The DataFrame index must be datetime-like for heatmap transformation.')
1214-1221: Plotly Pie:marker_colorsis invalid — colors won’t apply.
go.Pieexpectsmarker=dict(colors=[...]), notmarker_colors=.... This breaks coloring.Apply this diff:
- return go.Pie(- labels=labels,- values=values,- name=side,- marker_colors=trace_colors,- hole=hole,- textinfo=text_info,- textposition=text_position,- insidetextorientation='radial',- hovertemplate=hover_template,- sort=True, # Sort values by default (largest first)- )+ return go.Pie(+ labels=labels,+ values=values,+ name=side,+ marker=dict(colors=trace_colors),+ hole=hole,+ textinfo=text_info,+ textposition=text_position,+ insidetextorientation='radial',+ hovertemplate=hover_template,+ sort=True, # Sort values by default (largest first)+ )
🧹 Nitpick comments (9)
flixopt/results.py (1)
813-813: Fix misleading log message: references .nc4 but we load a .jsonThe log says we're loading an .nc4 file, yet the next line opens a .json. Align the message to avoid confusion.
Apply this diff:
- logger.info(f'loading calculation "{name}" from file ("{path.with_suffix(".nc4")}")')+ logger.info(f'loading calculation "{name}" from file ("{path.with_suffix(".json")}")')flixopt/plotting.py (8)
788-789: RaiseValueErrorfor unsupported (periods, steps) pairs.Matches public API expectations; please update the docstring accordingly (it still says “raise an assertion”).
Apply this docstring tweak:
- Only specific combinations of `periods` and `steps_per_period` are supported; invalid combinations raise an assertion.+ Only specific combinations of `periods` and `steps_per_period` are supported; invalid combinations raise a ValueError.
1125-1127: Doc clarity improvement.The clarified parameter docs for
holeandlower_percentage_groupare helpful. Consider also stating that settinglower_percentage_group=0disables grouping.
513-526: Bar width with datetime index may be mis-scaled.
ax.bar(..., width=<Timedelta>)can render oddly across Matplotlib versions. Convert to days explicitly for consistent sizing.Suggested change:
- width = data.index.to_series().diff().dropna().min() # Minimum time difference+ width = data.index.to_series().diff().dropna().min()+ if isinstance(width, pd.Timedelta):+ width = width.total_seconds() / 86400.0 # Matplotlib date units are days
535-536: Hide negative-bar artists from legend explicitly.Use
_nolegend_to ensure they never appear if legend logic changes.- label='', # No label for negative bars+ label='_nolegend_', # Exclude from legend explicitly
1441-1444:default_filetypemust include a leading dot.
Path.with_suffix()requires a dot; passing"png"raisesValueError. Normalize the input.Apply this diff:
- if filename.suffix == '':- if default_filetype is None:- raise ValueError('No default filetype provided')- filename = filename.with_suffix(default_filetype)+ if filename.suffix == '':+ if not default_filetype:+ raise ValueError('No default filetype provided')+ suffix = default_filetype if default_filetype.startswith('.') else f'.{default_filetype}'+ filename = filename.with_suffix(suffix)
1461-1461: Preferfig.show()over globalplt.show().
plt.show()displays all open figures;fig.show()scopes to the given figure.- if show:- plt.show()+ if show:+ try:+ fig.show()+ except Exception:+ plt.show()
706-721: Validatenr_of_steps_per_column> 0.Avoid division by zero and undefined reshape for invalid input.
+ if nr_of_steps_per_column <= 0:+ raise ValueError('nr_of_steps_per_column must be a positive integer')
197-206: Catching onlyPlotlyErrormay miss invalid colorscale errors.
px.colors.get_colorscalecan raiseValueError/KeyError. Broaden the except.- except PlotlyError as e:+ except Exception as e:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
flixopt/plotting.py(11 hunks)flixopt/results.py(1 hunks)
🔇 Additional comments (7)
flixopt/plotting.py (7)
265-269: Deterministic missing-label handling — nice.Sorting
missing_labelsguarantees stable color assignment across runs.
355-356: Replace assert with explicit validation — good move.Raising
ValueErrorfor invalid Plotly mode is appropriate for library code.
598-598:shading='auto'avoids QuadMesh shape pitfalls.This eliminates edge cases where cell counts mismatch x/y coordinates.
618-618: Avoids private attr_Aon ScalarMappable.
set_array([])is the correct, forward-compatible approach.
775-776: Early empty-DataFrame validation — good.Clear error signaling before heavy processing.
956-956: Unified color processing for Plotly pie — good.Using labels with
ColorProcessorensures cross-plot consistency.
1447-1450: Auto-switch to.htmlfor Plotly saves — sensible default.Prevents invalid export types and surprises.
Uh oh!
There was an error while loading. Please reload this page.
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit