Remove the plot customization dialog - #14034
Conversation
There was a problem hiding this comment.
Pull request overview
This PR redesigns plot styling/customization in the GUI by replacing the old “Customize plot” dialog + per-key PlotConfig history with a new sidebar-based style editor and a refactored, role-based plotting style model.
Changes:
- Replace the customization dialog workflow with new sidebar controls (
StylePlotOptions) and per-key title/label storage inPlotWindow. - Refactor plotting styles into
RenderMode/StyleRole+ a frozenPlotStyledataclass, and update plotters to consume Matplotlib keyword-style dictionaries. - Update/add unit tests for the new style controls and refactored plotting style behavior; remove tests tied to the deleted customization dialog/history.
Reviewed changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ert/unit_tests/gui/tools/plot/test_plot_window.py | Adjusts plot-window tests to work with new per-key plot text storage (title/labels). |
| tests/ert/unit_tests/gui/plotting/widgets/test_style_options.py | Adds unit coverage for the new sidebar style options widget (presets, reset, visibility). |
| tests/ert/unit_tests/gui/plottery/test_plot_style.py | Updates tests to validate the new PlotStyle/PlotConfig style-role behavior. |
| tests/ert/unit_tests/gui/plottery/test_plot_config_history.py | Removes tests for PlotConfigHistory (feature removed). |
| tests/ert/unit_tests/gui/plottery/test_history.py | Updates history plotting test assertions for the new style API (StyleRole + linewidth/linestyle fields). |
| tests/ert/ui_tests/gui/test_plot_customization.py | Removes UI test for the old customization dialog (feature removed). |
| src/ert/gui/plotting/widgets/plot_widget.py | Removes the toolbar “Customize” action and related signals. |
| src/ert/gui/plotting/widgets/plot_controls/style_options.py | Introduces the new sidebar style editor widget (line styles, statistics presets, distribution toggles). |
| src/ert/gui/plotting/widgets/plot_controls/observation_color.py | Switches ColorBox import to the new location under plot_controls. |
| src/ert/gui/plotting/widgets/plot_controls/custom_palette_dialog.py | Switches ColorBox import to the new location under plot_controls. |
| src/ert/gui/plotting/widgets/plot_controls/color_chooser.py | Adds the new ColorBox widget implementation under plot_controls. |
| src/ert/gui/plotting/widgets/plot_controls/init.py | Exports StylePlotOptions from the plot_controls package. |
| src/ert/gui/plotting/widgets/copy_style_to_dialog.py | Removes legacy “copy style to other keys” dialog (feature removed). |
| src/ert/gui/plotting/widgets/init.py | Removes CopyStyleToDialog export (file removed). |
| src/ert/gui/plotting/utils/qt_creator.py | Adds a new collapsible group-box helper for the sidebar sections. |
| src/ert/gui/plotting/utils/plot_style.py | Replaces legacy mutable PlotStyle with a frozen dataclass and introduces StyleRole/RenderMode + presets/defaults. |
| src/ert/gui/plotting/utils/plot_config.py | Refactors PlotConfig to be role-based and palette-resolving via style(role). |
| src/ert/gui/plotting/utils/plot_config_history.py | Removes PlotConfigHistory (feature removed). |
| src/ert/gui/plotting/utils/plot_config_factory.py | Removes PlotConfigFactory (feature removed). |
| src/ert/gui/plotting/utils/init.py | Updates exports to match the new style model and removes old factory/history exports. |
| src/ert/gui/plotting/shared_plots/ensemble.py | Updates ensemble plotting to use PlotStyle.line_properties() and StyleRole-based config. |
| src/ert/gui/plotting/plot_window.py | Integrates the new sidebar style options and replaces old PlotCustomizer usage with per-key text + style options. |
| src/ert/gui/plotting/ert_plots/statistics.py | Updates statistics plotting/legends to use StyleRole + RenderMode (area vs line) and Matplotlib keyword dicts. |
| src/ert/gui/plotting/ert_plots/observations.py | Updates observation plotting to use StyleRole and new style field names (linestyle/linewidth/markersize). |
| src/ert/gui/plotting/ert_plots/history.py | Updates history plotting to use StyleRole and line_properties(). |
| src/ert/gui/plotting/ert_plots/histogram.py | Updates histogram plotting to fetch the histogram role style via style(StyleRole.HISTOGRAM). |
| src/ert/gui/plotting/ert_plots/gaussian_kde.py | Updates KDE plotting to use style(StyleRole.HISTOGRAM) and new linewidth field name. |
| src/ert/gui/plotting/ert_plots/distribution.py | Updates distribution plotting to use StyleRole.DISTRIBUTION and StyleRole.DISTRIBUTION_LINE styles. |
| src/ert/gui/plotting/customization_dialog/style_customization_view.py | Removes legacy dialog customization view (feature removed). |
| src/ert/gui/plotting/customization_dialog/style_chooser.py | Removes legacy style chooser (feature removed). |
| src/ert/gui/plotting/customization_dialog/statistics_customization_view.py | Removes legacy statistics customization view (feature removed). |
| src/ert/gui/plotting/customization_dialog/customize_plot_dialog.py | Removes legacy customization dialog and PlotCustomizer implementation (feature removed). |
| src/ert/gui/plotting/customization_dialog/customization_view.py | Removes legacy customization base view (feature removed). |
| src/ert/gui/plotting/customization_dialog/init.py | Removes customization_dialog package exports (feature removed). |
| from ert.gui.plotting.utils.logging_utils import log_plot_option_usage_once | ||
| from ert.gui.plotting.utils.plot_config import PlotConfig | ||
| from ert.gui.plotting.widgets.plot_controls.color_chooser import ColorBox | ||
|
|
||
| logger = logging.getLogger(__name__) |
| from ert.gui.plotting.utils.logging_utils import log_plot_option_usage_once | ||
| from ert.gui.plotting.utils.plot_color_palettes import MINIMUM_COLOR_CYCLE_LENGTH | ||
| from ert.gui.plotting.widgets.plot_controls.color_chooser import ColorBox | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Nope, not making any changes to the colorbox usage at this point. It's probably from an earlier state of the PR where it was larger redesign.
a39fc70 to
dee8ca5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 34 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/ert/gui/plotting/widgets/plot_controls/style_options.py:408
StylePlotOptionsonly creates the statistics/distribution widgets whenis_everestis False, but methods/properties likeset_statistics_styles_visible(),set_distribution_styles_visible(),std_dev_factor, anddistribution_lines_checkbox_stateassume those attributes always exist. If any caller accesses these on an Everest instance, it will raiseAttributeErrorat runtime (easy footgun since the class is exported publicly). Consider guarding these APIs and returning sensible defaults when the sections are not present.
def set_statistics_styles_visible(self, visible: bool) -> None:
self._statistics_options.setVisible(visible)
def set_distribution_styles_visible(self, visible: bool) -> None:
self._distribution_options.setVisible(visible)
def styles(self) -> PlotStyles:
return {role: row.style() for role, row in self._rows.items()}
@property
def std_dev_factor(self) -> int:
return self._std_dev_factor.value()
@property
def distribution_lines_checkbox_state(self) -> bool:
return self._toggle_distribution_lines.isChecked()
416c766 to
08021d3
Compare
|
Screenshots differ from baselines. A baseline update PR has been prepared: equinor/ert-testdata#69 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14034 +/- ##
==========================================
+ Coverage 91.52% 91.78% +0.25%
==========================================
Files 488 480 -8
Lines 34092 33437 -655
==========================================
- Hits 31204 30689 -515
+ Misses 2888 2748 -140
Flags with carried forward coverage won't be shown. Click here to find out more.
|
08021d3 to
5b4d067
Compare
MagnusSletten
changed the title
5b4d067 to
79b0e25
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/ert/gui/plotting/plot_window.py:271
EnsembleSelectionWidgetis initialized withlen(TABLEAU_10_COLOR_CYCLE)(10), but the palette selector offers palettes with more colors (e.g. Color Brewer has 12). With the current hardcoded value, the extra colors can never be assigned to ensembles, so selecting a 12-color palette won’t actually use all available colors.
self._ensemble_selection_widget = EnsembleSelectionWidget(
plot_case_objects,
len(TABLEAU_10_COLOR_CYCLE),
)
The plot window previously copied its PlotConfig from PlotCustomizer. This also maintained a PlotConfigHistory for each key, allowing styling, titles, and axis labels to be edited and undone through ERT's customization dialog. After removing that dialog, only titles and axis labels require per-key state. Store these overrides directly on the plot window in three dictionaries keyed by plot key. A missing title falls back to the key name, while missing axis labels remain None so each plotter can provide its own defaults. Rebuild the PlotConfig from PlotConfigFactory on every redraw. This preserves the existing statistics defaults for each plot dimensionality.
79b0e25 to
4da1f3b
Compare
|
Havent had time to review yet, but if I remember correctly from the meeting yesterday, did we not conclude to not opt-in for the |
It lacks proper handling of categories for lines and so the line customization is mostly useless. I can remove it. Title/axis labels is already handled by our implementation as well. |
ERT replaced Matplotlib's Customize toolbar action with a gear button opening its own dialog. With that dialog gone the gear no longer does anything, so remove it. Matplotlib's own action is removed alongside it, leaving no customization entry in the toolbar until the sidebar controls land.
4da1f3b to
69bf6af
Compare
eilskra
added
the
release-notes:skip
There was a problem hiding this comment.
Looks very good, just a few comments for clarifications 👍 Outside of that, some considerations:
- Have you ensure that the deleted files did not have any unforeseen other usage?
- Does change observation color etc work as normal?
- Bar unit-testing, has there been any manual testing of
heat_equationandsnake_oilto ensure that things work as intended? - Is Everest unaffected?
| from ert.gui.plotting.utils.logging_utils import log_plot_option_usage_once | ||
| from ert.gui.plotting.utils.plot_color_palettes import MINIMUM_COLOR_CYCLE_LENGTH | ||
| from ert.gui.plotting.widgets.plot_controls.color_chooser import ColorBox | ||
|
|
||
| logger = logging.getLogger(__name__) |
| self._plot_customizer = PlotCustomizer(self, self._key_definitions) | ||
| self._plot_customizer.settingsChanged.connect(self.keySelected) | ||
| self._titles: dict[str, str] = {} | ||
| self._x_labels: dict[str, str | None] = {} |
There was a problem hiding this comment.
Iirc the previous default was "Unnamed". Unsure if this is something we should keep or change. Thoughts?
There was a problem hiding this comment.
The default is "Unnamed" from the finalize_plots method itself but it's never actually used. All the finalize_plots calls use custom ones passed as parameters, e.g:
default_x_label = "Date" if plot_context.is_date_support_active() else "Index"
...
PlotTools.finalizePlot(
plot_context,
figure,
axes,
default_x_label=default_x_label,
default_y_label="Value",
)
So there's currently no change in behaviour when the labels are None.
| key_def = self.getSelectedKey() | ||
| if key_def is None: | ||
| return |
There was a problem hiding this comment.
(method) def getSelectedKey(self: Self@PlotWindow) -> (PlotApiKeyDefinition | None)
At least according to the declared typing. I can check this behaviour though. The none-checks was also there prior to this PR.
| else: | ||
| plot_config.set_y_label(new_label) | ||
| self._plot_customizer.update_plot_config(plot_config) | ||
| labels[key_def.key] = new_label |
There was a problem hiding this comment.
Do we set it as None to trigger the default name?
There was a problem hiding this comment.
Yep!
Essentially because of this code in plot_tools.py:
if config.x_label() is None:
config.set_x_label(default_x_label)
| self.update_plot() | ||
|
|
||
| def _edit_title(self) -> None: | ||
| key_def = self.getSelectedKey() |
There was a problem hiding this comment.
Similar question as on R673-675, if needed, should be extracted since repeated code
There was a problem hiding this comment.
Early return here cannot easily be extracted. The extraction would also take more lines of code than having the repeats of None-checks here. It can be shorter though, e.g
if (key_def := self.getSelectedKey()) is None:
return
| plot_config = plot_window._plot_customizer.get_plot_config() | ||
| plot_config.set_title("Custom title") | ||
| plot_window._plot_customizer.update_plot_config(plot_config) | ||
| plot_window._titles["some_key"] = "Custom title" |
There was a problem hiding this comment.
Why are we not using the plot_config to set title here? Why are we overwriting the dictionary?
There was a problem hiding this comment.
There's currently no reachable plot_config after the removal of plot_customizer. The dict is what persists titles/labels for each key. I think we can improve on plot_config handling in another PR.
eqbech
left a comment
eqbech
left a comment
There was a problem hiding this comment.
Looks good, tested locally using poly_example. I did however notice that the legend option under general options did not toggle legends under the histogram plot? Has it always been that way or is that something introduced here?
Prior to this PR the only use-case for it was the style customization for the elements still in the customizer. If per-key configuration of plots is wanted again in the future, this code would be useful, but that's still recoverable from history. It's probably better to rewrite it at that point as well. Deleted: customization_view.py - purely visual builder for customize_plot_dialog The one larger consequence is temporary and that's the lack of statistics customization.
Yep!
Very frequent manual testing in snake_oil, but it does not hurt to have another manual test here if you want to quickly do it. |
Happens on main as well @eqbech. There's some custom legend handling there, but that whole class is getting completely changed by upcoming pr #13985 |
Issue
Resolves #14021
Approach
This removes ERT's custom plot customization code, the dialog behind the pencil icon, its per-key config history and copy-between-keys support.
Per-key titles and axis labels move to dicts on the plot window and stay editable.
Statistics temporarily lose more than styling: because selection was expressed through line styles, choosing which statistics are drawn goes away too, along with the std dev multiplier and distribution connection lines.
These return in a follow-up PR as explicit toggles, decoupled from styling.
Visible changes:
No edit icon from customization dialog visiible:
git rebase -i main --exec 'just rapid-tests')When applicable
merge screenshot-PR in ert-testdata before merging this PR.