Feature/aggregate rework v2+plotting #546
Conversation
…ation, reducing code duplication while maintaining the same functionality (data preparation, color resolution from components, PlotResult wrapping).
… area(), and duration_curve() methods in both DatasetPlotAccessor and DataArrayPlotAccessor
2. scatter() method - Plots two variables against each other with x and y parameters
3. pie() method - Creates pie charts from aggregated (scalar) dataset values, e.g. ds.sum('time').fxplot.pie()
4. duration_curve() method - Sorts values along the time dimension in descending order, with optional normalize parameter for percentage x-axis
5. CONFIG.Plotting.default_line_shape - New config option (default 'hv') that controls the default line shape for line(), area(), and duration_curve() methods
1. X-axis is now determined first using CONFIG.Plotting.x_dim_priority
2. Facets are resolved from remaining dimensions (x-axis excluded)
x_dim_priority expanded:
x_dim_priority = ('time', 'duration', 'duration_pct', 'period', 'scenario', 'cluster')
- Time-like dims first, then common grouping dims as fallback
- variable stays excluded (it's used for color, not x-axis)
_get_x_dim() refactored:
- Now takes dims: list[str] instead of a DataFrame
- More versatile - works with any list of dimension names
- Add `x` parameter to bar/stacked_bar/line/area for explicit x-axis control - Add CONFIG.Plotting.x_dim_priority for auto x-axis selection order - X-axis determined first, facets from remaining dimensions - Refactor _get_x_column -> _get_x_dim (takes dim list, not DataFrame) - Support scalar data (no dims) by using 'variable' as x-axis
- Add `x` parameter to bar/stacked_bar/line/area for explicit x-axis control
- Add CONFIG.Plotting.x_dim_priority for auto x-axis selection
Default: ('time', 'duration', 'duration_pct', 'period', 'scenario', 'cluster')
- X-axis determined first, facets resolved from remaining dimensions
- Refactor _get_x_column -> _get_x_dim (takes dim list, more versatile)
- Support scalar data (no dims) by using 'variable' as x-axis
- Skip color='variable' when x='variable' to avoid double encoding
- Fix _dataset_to_long_df to use dims (not just coords) as id_vars
- Add `x` parameter to bar/stacked_bar/line/area for explicit x-axis control
- Add CONFIG.Plotting.x_dim_priority for auto x-axis selection
Default: ('time', 'duration', 'duration_pct', 'period', 'scenario', 'cluster')
- X-axis determined first, facets resolved from remaining dimensions
- Refactor _get_x_column -> _get_x_dim (takes dim list, more versatile)
- Support scalar data (no dims) by using 'variable' as x-axis
- Skip color='variable' when x='variable' to avoid double encoding
- Fix _dataset_to_long_df to use dims (not just coords) as id_vars
- Ensure px_kwargs properly overrides all defaults (color, facets, etc.)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughIntroduces xarray accessor extensions ( Changes
Sequence DiagramsequenceDiagram
participant User
participant Accessor as DatasetPlotAccessor
participant Helper as Dimension/Facet Helpers
participant DataFrame as DataFrame Converter
participant PlotlyExpress as Plotly Express
participant Figure as go.Figure
User->>Accessor: .fxplot.line(x='auto', facet_col='auto', ...)
Accessor->>Helper: _get_x_dim(ds, x_priority)
Helper-->>Accessor: x_dim resolved
Accessor->>Helper: _resolve_auto_facets(ds, dims, priority)
Helper-->>Accessor: facet_col, facet_row, animation_frame
Accessor->>Accessor: Apply color mapping & styling
Accessor->>DataFrame: _dataset_to_long_df(ds, x_dim)
DataFrame-->>Accessor: long-form DataFrame
Accessor->>PlotlyExpress: px.line(df, x, y, facet_col, ...)
PlotlyExpress-->>Figure: Plotly Figure
Accessor->>Figure: Update layout (titles, labels, wrapping)
Figure-->>User: return go.Figure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
flixopt/config.py (1)
592-595: LGTM!Both attributes are properly initialized from the _DEFAULTS mapping, following the existing pattern for configuration attributes.
Optional: Add type hints for new attributes
For consistency with some other CONFIG attributes and improved IDE support, consider adding type hints:
- default_line_shape: str = _DEFAULTS['plotting']['default_line_shape'] + default_line_shape: str = _DEFAULTS['plotting']['default_line_shape'] # Already has type hint extra_dim_priority: tuple[str, ...] = _DEFAULTS['plotting']['extra_dim_priority'] dim_slot_priority: tuple[str, ...] = _DEFAULTS['plotting']['dim_slot_priority'] - x_dim_priority: tuple[str, ...] = _DEFAULTS['plotting']['x_dim_priority'] + x_dim_priority: tuple[str, ...] = _DEFAULTS['plotting']['x_dim_priority'] # Already has type hintActually, both already have type hints! No changes needed.
flixopt/dataset_plot_accessor.py (1)
30-68: Consider consolidating withflixopt/statistics_accessor.py.This function largely duplicates
_resolve_auto_facetsinstatistics_accessor.py(lines 182-233 per the relevant snippets), with the addition of anexclude_dimsparameter. Consider extracting a shared utility to reduce duplication. The implementation itself is correct.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/notebooks/fxplot_accessor_demo.ipynbdocs/user-guide/recipes/plotting-custom-data.mdflixopt/__init__.pyflixopt/config.pyflixopt/dataset_plot_accessor.pyflixopt/statistics_accessor.pymkdocs.yml
🧰 Additional context used
🧬 Code graph analysis (2)
flixopt/statistics_accessor.py (2)
flixopt/dataset_plot_accessor.py (6)
stacked_bar(192-268)stacked_bar(756-782)heatmap(423-487)heatmap(844-894)line(270-346)line(784-812)flixopt/clustering/base.py (1)
heatmap(660-794)
flixopt/dataset_plot_accessor.py (4)
flixopt/color_processing.py (1)
process_colors(165-233)flixopt/config.py (2)
CONFIG(185-916)Plotting(551-595)flixopt/statistics_accessor.py (6)
_resolve_auto_facets(183-234)sizes(464-468)sizes(1035-1078)sizes(1766-1830)_dataset_to_long_df(250-260)heatmap(1539-1669)flixopt/plot_result.py (1)
update_traces(75-88)
⏰ 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.13)
- GitHub Check: test (3.14)
🔇 Additional comments (20)
mkdocs.yml (1)
76-76: LGTM!The navigation entry for the new plotting demo notebook is properly formatted and well-placed in the Examples section.
flixopt/statistics_accessor.py (3)
1656-1661: LGTM!The delegation to
da.fxplot.heatmap()is appropriate. The fxplot accessor is registered for both Dataset and DataArray per the PR changes.
2145-2152: LGTM!The refactoring to use fxplot accessor methods consolidates plotting logic effectively. All delegations follow a consistent pattern and preserve the original functionality while improving maintainability.
1399-1406: Accessor registration is properly handled.The fxplot accessor is registered globally via
@xr.register_dataset_accessor('fxplot')indataset_plot_accessor.py, and the registration is ensured by the unconditional import inflixopt/__init__.py(line 19). This guarantees the accessor is available whenever flixopt is imported through its standard package interface. The typical usage path—accessing statistics throughFlowSystem.statistics—will always have the accessor registered, so no defensive checks are needed.flixopt/__init__.py (1)
17-19: LGTM!The accessor registration pattern is correctly implemented. Importing
dataset_plot_accessortriggers the@xr.register_dataset_accessorand@xr.register_dataarray_accessordecorators, making.fxplotand.fxstatsavailable on xarray objects. The use ofas _andnoqa: F401are standard practices for side-effect imports.docs/user-guide/recipes/plotting-custom-data.md (1)
1-30: LGTM!The documentation update effectively introduces the fxplot accessor with a clear quick-start example and proper reference to comprehensive documentation. The structure (intro → quick example → full docs pointer) provides a good learning path for users.
flixopt/config.py (5)
166-166: LGTM!The
default_line_shape: 'hv'setting is appropriate for energy system time series where values typically remain constant within time steps (creating step-like plots with horizontal segments first).
169-169: LGTM!The
x_dim_prioritytuple establishes a sensible default for automatic x-axis selection, prioritizing time-based dimensions before structural dimensions.
569-570: LGTM!The docstring addition clearly documents the x_dim_priority attribute and its purpose.
696-699: LGTM!Both new attributes are properly included in the
to_dict()serialization, ensuring they can be saved and restored with the configuration.
166-699: Configuration additions are well-integrated.Both
default_line_shapeandx_dim_priorityare properly integrated across all necessary locations:
- Immutable defaults (_DEFAULTS)
- Class attributes (CONFIG.Plotting)
- Serialization (to_dict)
- Documentation (docstrings)
The implementation is consistent with existing configuration patterns.
docs/notebooks/fxplot_accessor_demo.ipynb (1)
1-565: Well-structured demo notebook covering accessor functionality comprehensively.The notebook effectively demonstrates the
.fxplotaccessor with clear sections for each plot type, faceting/animation features, custom colors, axis labels, and chaining with Plotly methods. The examples are self-contained and educational.flixopt/dataset_plot_accessor.py (8)
16-27: LGTM!The
_get_x_dimhelper correctly handles explicit dimension selection, priority-based auto-selection, and graceful fallback for scalar data.
71-81: LGTM!The helper correctly handles empty datasets, scalar datasets (creating explicit rows), and multi-dimensional datasets using
melt(). Theid_colsfiltering ensures dims without coordinates are handled properly.
119-190: LGTM!The
barmethod correctly handles x-axis selection, facet resolution (excluding x dimension), empty data, and avoids double-encoding whenx='variable'. Thepx_kwargsmerge at the end properly allows user overrides.
192-268: LGTM!The
stacked_barmethod correctly usesbarmode='relative'for proper positive/negative stacking, with appropriate styling (bargap=0,marker_line_width=0) for a clean stacked appearance.
270-346: LGTM!The
linemethod properly supports configurableline_shapewith a sensible fallback toCONFIG.Plotting.default_line_shape. Faceting and animation logic is consistent with other plotting methods.
348-421: LGTM!The
areamethod follows the same consistent pattern asline, with properline_shapesupport.
559-639: LGTM!The
piemethod correctly handles both scalar (single pie) and multi-dimensional (faceted pies) cases. Color mapping is properly applied, andpx_kwargsis correctly spread to allow user overrides.
700-893: DataArrayPlotAccessor correctly proxies to Dataset methods.The design of excluding
scatter(requires two variables) andpie(works on aggregated multi-variable data) fromDataArrayPlotAccessoris reasonable. The proxy pattern with_to_dataset()is clean.
flixopt/dataset_plot_accessor.py
Outdated
| imshow_args: dict[str, Any] = { | ||
| 'img': da, | ||
| 'color_continuous_scale': colors, | ||
| 'title': title or variable, | ||
| **imshow_kwargs, | ||
| } | ||
|
|
||
| if actual_facet_col and actual_facet_col in da.dims: | ||
| imshow_args['facet_col'] = actual_facet_col | ||
| if facet_col_wrap < da.sizes[actual_facet_col]: | ||
| imshow_args['facet_col_wrap'] = facet_col_wrap | ||
|
|
||
| if actual_anim and actual_anim in da.dims: | ||
| imshow_args['animation_frame'] = actual_anim | ||
|
|
||
| return px.imshow(**imshow_args) |
There was a problem hiding this comment.
Inconsistent kwarg merge order prevents user overrides.
In heatmap, imshow_kwargs is spread before hardcoded values ('color_continuous_scale': colors, 'title': title or variable), preventing users from overriding these via imshow_kwargs. Other methods like bar (line 190) correctly place **px_kwargs last.
🔎 Proposed fix
imshow_args: dict[str, Any] = {
'img': da,
'color_continuous_scale': colors,
'title': title or variable,
- **imshow_kwargs,
}
if actual_facet_col and actual_facet_col in da.dims:
imshow_args['facet_col'] = actual_facet_col
if facet_col_wrap < da.sizes[actual_facet_col]:
imshow_args['facet_col_wrap'] = facet_col_wrap
if actual_anim and actual_anim in da.dims:
imshow_args['animation_frame'] = actual_anim
- return px.imshow(**imshow_args)
+ return px.imshow(**{**imshow_args, **imshow_kwargs})🤖 Prompt for AI Agents
In flixopt/dataset_plot_accessor.py around lines 472-487, the heatmap
imshow_kwargs are merged before the hardcoded defaults which prevents users from
overriding values like color_continuous_scale and title; change the merge order
so imshow_kwargs are applied last (keep 'img' set first), and when adding
facet_col/facet_col_wrap and animation_frame either set them before merging or
rely on the final merge so user-supplied keys in imshow_kwargs can override the
defaults; ensure the final dict passed to px.imshow has **imshow_kwargs last.
| def scatter( | ||
| self, | ||
| x: str, | ||
| y: str, | ||
| *, | ||
| colors: ColorType | None = None, | ||
| title: str = '', | ||
| xlabel: str = '', | ||
| ylabel: str = '', | ||
| facet_col: str | Literal['auto'] | None = 'auto', | ||
| facet_row: str | Literal['auto'] | None = 'auto', | ||
| animation_frame: str | Literal['auto'] | None = 'auto', | ||
| facet_cols: int | None = None, | ||
| **px_kwargs: Any, | ||
| ) -> go.Figure: | ||
| """Create a scatter plot from two variables in the dataset. | ||
|
|
||
| Args: | ||
| x: Variable name for x-axis. | ||
| y: Variable name for y-axis. | ||
| colors: Color specification (colorscale name, color list, or dict mapping). | ||
| title: Plot title. | ||
| xlabel: X-axis label. | ||
| ylabel: Y-axis label. | ||
| facet_col: Dimension for column facets. 'auto' uses CONFIG priority. | ||
| facet_row: Dimension for row facets. | ||
| animation_frame: Dimension for animation slider. | ||
| facet_cols: Number of columns in facet grid wrap. | ||
| **px_kwargs: Additional arguments passed to plotly.express.scatter. | ||
|
|
||
| Returns: | ||
| Plotly Figure. | ||
| """ | ||
| if x not in self._ds.data_vars: | ||
| raise ValueError(f"Variable '{x}' not found in dataset. Available: {list(self._ds.data_vars)}") | ||
| if y not in self._ds.data_vars: | ||
| raise ValueError(f"Variable '{y}' not found in dataset. Available: {list(self._ds.data_vars)}") | ||
|
|
||
| df = self._ds[[x, y]].to_dataframe().reset_index() | ||
| if df.empty: | ||
| return go.Figure() | ||
|
|
||
| actual_facet_col, actual_facet_row, actual_anim = _resolve_auto_facets( | ||
| self._ds, facet_col, facet_row, animation_frame | ||
| ) | ||
|
|
||
| facet_col_wrap = facet_cols or CONFIG.Plotting.default_facet_cols | ||
| fig_kwargs: dict[str, Any] = { | ||
| 'data_frame': df, | ||
| 'x': x, | ||
| 'y': y, | ||
| 'title': title, | ||
| **px_kwargs, | ||
| } | ||
| if xlabel: | ||
| fig_kwargs['labels'] = {**fig_kwargs.get('labels', {}), x: xlabel} | ||
| if ylabel: | ||
| fig_kwargs['labels'] = {**fig_kwargs.get('labels', {}), y: ylabel} | ||
|
|
||
| if actual_facet_col: | ||
| fig_kwargs['facet_col'] = actual_facet_col | ||
| if facet_col_wrap < self._ds.sizes.get(actual_facet_col, facet_col_wrap + 1): | ||
| fig_kwargs['facet_col_wrap'] = facet_col_wrap | ||
| if actual_facet_row: | ||
| fig_kwargs['facet_row'] = actual_facet_row | ||
| if actual_anim: | ||
| fig_kwargs['animation_frame'] = actual_anim | ||
|
|
||
| return px.scatter(**fig_kwargs) |
There was a problem hiding this comment.
The colors parameter is accepted but never used.
The scatter method signature includes colors: ColorType | None = None (line 494), but this parameter is never applied in the method body. The color mapping is not passed to px.scatter.
🔎 Proposed fix: Either remove unused parameter or implement color support
Option 1 - Remove unused parameter:
def scatter(
self,
x: str,
y: str,
*,
- colors: ColorType | None = None,
title: str = '',
xlabel: str = '',Option 2 - Implement color support (e.g., for a third variable):
fig_kwargs: dict[str, Any] = {
'data_frame': df,
'x': x,
'y': y,
'title': title,
- **px_kwargs,
}
+ # Apply color if user specifies via px_kwargs
+ # (colors param could be used for color_discrete_map if 'color' is set)
+ if 'color' in px_kwargs and colors:
+ color_vals = df[px_kwargs['color']].unique().tolist()
+ color_map = process_colors(colors, color_vals, default_colorscale=CONFIG.Plotting.default_qualitative_colorscale)
+ fig_kwargs['color_discrete_map'] = color_map📝 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.
| def scatter( | |
| self, | |
| x: str, | |
| y: str, | |
| *, | |
| colors: ColorType | None = None, | |
| title: str = '', | |
| xlabel: str = '', | |
| ylabel: str = '', | |
| facet_col: str | Literal['auto'] | None = 'auto', | |
| facet_row: str | Literal['auto'] | None = 'auto', | |
| animation_frame: str | Literal['auto'] | None = 'auto', | |
| facet_cols: int | None = None, | |
| **px_kwargs: Any, | |
| ) -> go.Figure: | |
| """Create a scatter plot from two variables in the dataset. | |
| Args: | |
| x: Variable name for x-axis. | |
| y: Variable name for y-axis. | |
| colors: Color specification (colorscale name, color list, or dict mapping). | |
| title: Plot title. | |
| xlabel: X-axis label. | |
| ylabel: Y-axis label. | |
| facet_col: Dimension for column facets. 'auto' uses CONFIG priority. | |
| facet_row: Dimension for row facets. | |
| animation_frame: Dimension for animation slider. | |
| facet_cols: Number of columns in facet grid wrap. | |
| **px_kwargs: Additional arguments passed to plotly.express.scatter. | |
| Returns: | |
| Plotly Figure. | |
| """ | |
| if x not in self._ds.data_vars: | |
| raise ValueError(f"Variable '{x}' not found in dataset. Available: {list(self._ds.data_vars)}") | |
| if y not in self._ds.data_vars: | |
| raise ValueError(f"Variable '{y}' not found in dataset. Available: {list(self._ds.data_vars)}") | |
| df = self._ds[[x, y]].to_dataframe().reset_index() | |
| if df.empty: | |
| return go.Figure() | |
| actual_facet_col, actual_facet_row, actual_anim = _resolve_auto_facets( | |
| self._ds, facet_col, facet_row, animation_frame | |
| ) | |
| facet_col_wrap = facet_cols or CONFIG.Plotting.default_facet_cols | |
| fig_kwargs: dict[str, Any] = { | |
| 'data_frame': df, | |
| 'x': x, | |
| 'y': y, | |
| 'title': title, | |
| **px_kwargs, | |
| } | |
| if xlabel: | |
| fig_kwargs['labels'] = {**fig_kwargs.get('labels', {}), x: xlabel} | |
| if ylabel: | |
| fig_kwargs['labels'] = {**fig_kwargs.get('labels', {}), y: ylabel} | |
| if actual_facet_col: | |
| fig_kwargs['facet_col'] = actual_facet_col | |
| if facet_col_wrap < self._ds.sizes.get(actual_facet_col, facet_col_wrap + 1): | |
| fig_kwargs['facet_col_wrap'] = facet_col_wrap | |
| if actual_facet_row: | |
| fig_kwargs['facet_row'] = actual_facet_row | |
| if actual_anim: | |
| fig_kwargs['animation_frame'] = actual_anim | |
| return px.scatter(**fig_kwargs) | |
| def scatter( | |
| self, | |
| x: str, | |
| y: str, | |
| *, | |
| title: str = '', | |
| xlabel: str = '', | |
| ylabel: str = '', | |
| facet_col: str | Literal['auto'] | None = 'auto', | |
| facet_row: str | Literal['auto'] | None = 'auto', | |
| animation_frame: str | Literal['auto'] | None = 'auto', | |
| facet_cols: int | None = None, | |
| **px_kwargs: Any, | |
| ) -> go.Figure: | |
| """Create a scatter plot from two variables in the dataset. | |
| Args: | |
| x: Variable name for x-axis. | |
| y: Variable name for y-axis. | |
| title: Plot title. | |
| xlabel: X-axis label. | |
| ylabel: Y-axis label. | |
| facet_col: Dimension for column facets. 'auto' uses CONFIG priority. | |
| facet_row: Dimension for row facets. | |
| animation_frame: Dimension for animation slider. | |
| facet_cols: Number of columns in facet grid wrap. | |
| **px_kwargs: Additional arguments passed to plotly.express.scatter. | |
| Returns: | |
| Plotly Figure. | |
| """ | |
| if x not in self._ds.data_vars: | |
| raise ValueError(f"Variable '{x}' not found in dataset. Available: {list(self._ds.data_vars)}") | |
| if y not in self._ds.data_vars: | |
| raise ValueError(f"Variable '{y}' not found in dataset. Available: {list(self._ds.data_vars)}") | |
| df = self._ds[[x, y]].to_dataframe().reset_index() | |
| if df.empty: | |
| return go.Figure() | |
| actual_facet_col, actual_facet_row, actual_anim = _resolve_auto_facets( | |
| self._ds, facet_col, facet_row, animation_frame | |
| ) | |
| facet_col_wrap = facet_cols or CONFIG.Plotting.default_facet_cols | |
| fig_kwargs: dict[str, Any] = { | |
| 'data_frame': df, | |
| 'x': x, | |
| 'y': y, | |
| 'title': title, | |
| **px_kwargs, | |
| } | |
| if xlabel: | |
| fig_kwargs['labels'] = {**fig_kwargs.get('labels', {}), x: xlabel} | |
| if ylabel: | |
| fig_kwargs['labels'] = {**fig_kwargs.get('labels', {}), y: ylabel} | |
| if actual_facet_col: | |
| fig_kwargs['facet_col'] = actual_facet_col | |
| if facet_col_wrap < self._ds.sizes.get(actual_facet_col, facet_col_wrap + 1): | |
| fig_kwargs['facet_col_wrap'] = facet_col_wrap | |
| if actual_facet_row: | |
| fig_kwargs['facet_row'] = actual_facet_row | |
| if actual_anim: | |
| fig_kwargs['animation_frame'] = actual_anim | |
| return px.scatter(**fig_kwargs) |
🤖 Prompt for AI Agents
In flixopt/dataset_plot_accessor.py around lines 489-557 the scatter method
accepts a colors parameter but never uses it; either remove the unused parameter
or wire it into plotly express by adding it to the px kwargs (e.g., set
fig_kwargs['color'] = colors when colors is not None and, if colors is a mapping
or list for categorical coloring, ensure labels/legend keys are preserved), and
update any related docstring/type hints accordingly so the color argument
actually controls px.scatter coloring.
| # Sort each variable along time dimension (descending) | ||
| sorted_ds = self._ds.copy() | ||
| for var in sorted_ds.data_vars: | ||
| da = sorted_ds[var] | ||
| # Sort along time axis (descending) | ||
| sorted_values = np.sort(da.values, axis=da.dims.index('time'))[::-1] | ||
| sorted_ds[var] = (da.dims, sorted_values) |
There was a problem hiding this comment.
Sorting logic may fail for multi-dimensional arrays when time is not the first axis.
Line 683 uses [::-1] to reverse after np.sort, but np.sort with axis= returns the sorted array along that axis, and [::-1] only reverses the first axis. If time is not dimension 0, the reversal will be incorrect.
🔎 Proposed fix using np.flip with explicit axis
for var in sorted_ds.data_vars:
da = sorted_ds[var]
# Sort along time axis (descending)
- sorted_values = np.sort(da.values, axis=da.dims.index('time'))[::-1]
+ time_axis = da.dims.index('time')
+ sorted_values = np.flip(np.sort(da.values, axis=time_axis), axis=time_axis)
sorted_ds[var] = (da.dims, sorted_values)📝 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.
| # Sort each variable along time dimension (descending) | |
| sorted_ds = self._ds.copy() | |
| for var in sorted_ds.data_vars: | |
| da = sorted_ds[var] | |
| # Sort along time axis (descending) | |
| sorted_values = np.sort(da.values, axis=da.dims.index('time'))[::-1] | |
| sorted_ds[var] = (da.dims, sorted_values) | |
| # Sort each variable along time dimension (descending) | |
| sorted_ds = self._ds.copy() | |
| for var in sorted_ds.data_vars: | |
| da = sorted_ds[var] | |
| # Sort along time axis (descending) | |
| time_axis = da.dims.index('time') | |
| sorted_values = np.flip(np.sort(da.values, axis=time_axis), axis=time_axis) | |
| sorted_ds[var] = (da.dims, sorted_values) |
🤖 Prompt for AI Agents
In flixopt/dataset_plot_accessor.py around lines 678 to 684, the current
reversal uses [::-1] which only flips the first axis and will be wrong when
'time' is not axis 0; compute the integer axis index with axis_idx =
da.dims.index('time') (as already used for np.sort), then replace the post-sort
reversal with np.flip(sorted_values, axis=axis_idx) so you sort along the
correct axis and then reverse along that same axis before assigning back to
sorted_ds[var].
| imshow_args: dict[str, Any] = { | ||
| 'img': da, | ||
| 'color_continuous_scale': colors, | ||
| 'title': title or (da.name if da.name else ''), | ||
| **imshow_kwargs, | ||
| } | ||
|
|
||
| if actual_facet_col and actual_facet_col in da.dims: | ||
| imshow_args['facet_col'] = actual_facet_col | ||
| if facet_col_wrap < da.sizes[actual_facet_col]: | ||
| imshow_args['facet_col_wrap'] = facet_col_wrap | ||
|
|
||
| if actual_anim and actual_anim in da.dims: | ||
| imshow_args['animation_frame'] = actual_anim | ||
|
|
There was a problem hiding this comment.
Same kwarg merge order issue as DatasetPlotAccessor.heatmap.
The imshow_kwargs is spread inside the dict before being used, preventing user overrides of color_continuous_scale and title. Apply the same fix as suggested for DatasetPlotAccessor.heatmap.
🔎 Proposed fix
imshow_args: dict[str, Any] = {
'img': da,
'color_continuous_scale': colors,
'title': title or (da.name if da.name else ''),
- **imshow_kwargs,
}
if actual_facet_col and actual_facet_col in da.dims:
imshow_args['facet_col'] = actual_facet_col
if facet_col_wrap < da.sizes[actual_facet_col]:
imshow_args['facet_col_wrap'] = facet_col_wrap
if actual_anim and actual_anim in da.dims:
imshow_args['animation_frame'] = actual_anim
- return px.imshow(**imshow_args)
+ return px.imshow(**{**imshow_args, **imshow_kwargs})🤖 Prompt for AI Agents
In flixopt/dataset_plot_accessor.py around lines 879 to 893, the imshow_kwargs
are merged into imshow_args before default keys like 'color_continuous_scale'
and 'title', which prevents user-provided kwargs from overriding those defaults;
move the merge so user kwargs are applied last (e.g., construct imshow_args with
defaults and dimension-based keys first, then update imshow_args with
imshow_kwargs or spread **imshow_kwargs at the end) so user-specified
'color_continuous_scale' and 'title' take precedence while preserving the
existing facet_col/facet_col_wrap and animation_frame logic.
…wargs} so user can override 2. scatter unused colors - Removed the unused parameter 3. to_duration_curve sorting - Changed [::-1] to np.flip(..., axis=time_axis) for correct multi-dimensional handling 4. DataArrayPlotAccessor.heatmap - Same kwarg merge fix
Description
Add xarray accessors for plotting of all datasets and dataarrays!
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
.fxplotaccessor for easy visualization of datasets and arrays with bar, stacked bar, line, area, heatmap, scatter, and pie charts..fxstatsaccessor with duration curve transformation.Documentation
✏️ Tip: You can customize this high-level summary in your review settings.