Skip to content

Add controls for configuring localization update strategies - #14316

Open
frode-aarstad wants to merge 17 commits into
equinor:mainfrom
frode-aarstad:update-edit-settings-with-update-strategies
Open

Add controls for configuring localization update strategies#14316
frode-aarstad wants to merge 17 commits into
equinor:mainfrom
frode-aarstad:update-edit-settings-with-update-strategies

Conversation

@frode-aarstad

@frode-aarstad frode-aarstad commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Issue
Resolves #14296

Approach
Add controls for configuring localization update strategies per parameter type. Persist accepted threshold, truncation, and strategy changes while discarding edits when the dialog is cancelled and add GUI tests covering panel initialization and dialog behavior.

image
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'just rapid-tests')

When applicable

  • When screenshots are changed: Review screenshot-PR in ert-testdata,
    merge screenshot-PR in ert-testdata before merging this PR.
  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Add backport label to latest release (format: 'backport release-branch-name')

@frode-aarstad frode-aarstad self-assigned this Aug 28, 2026
@frode-aarstad
frode-aarstad marked this pull request as draft August 28, 2026 12:08
@frode-aarstad
frode-aarstad force-pushed the update-edit-settings-with-update-strategies branch from 29f2124 to 8ca3267 Compare August 28, 2026 13:27
@oddvarlia

Copy link
Copy Markdown
Contributor

I have some comments to this change. See my comments in issue: #14296

@frode-aarstad
frode-aarstad force-pushed the update-edit-settings-with-update-strategies branch from 4561d20 to 0059876 Compare August 31, 2026 11:51
@frode-aarstad
frode-aarstad force-pushed the update-edit-settings-with-update-strategies branch from 0059876 to cbbd2d8 Compare September 2, 2026 10:47
@frode-aarstad
frode-aarstad force-pushed the update-edit-settings-with-update-strategies branch from cbbd2d8 to fe3640e Compare September 2, 2026 12:14
@codecov-commenter

codecov-commenter commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 38.05970% with 83 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.89%. Comparing base (9d22460) to head (d0ab84e).
⚠️ Report is 35 commits behind head on main.

Files with missing lines Patch % Lines
...ert/gui/ertwidgets/analysismodulevariablespanel.py 24.56% 43 Missing ⚠️
src/ert/gui/ertwidgets/analysismoduleedit.py 26.92% 38 Missing ⚠️
src/ert/config/analysis_module.py 0.00% 1 Missing ⚠️
src/ert/run_models/run_model_configs.py 75.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (9d22460) and HEAD (d0ab84e). Click for more details.

HEAD has 10 uploads less than BASE
Flag BASE (9d22460) HEAD (d0ab84e)
gui-tests 4 3
test 4 3
performance-and-unit-tests 4 0
cli-tests 4 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main   #14316       +/-   ##
===========================================
- Coverage   91.75%   70.89%   -20.86%     
===========================================
  Files         485      495       +10     
  Lines       34719    36195     +1476     
===========================================
- Hits        31855    25661     -6194     
- Misses       2864    10534     +7670     
Flag Coverage Δ
cli-tests ?
fuzz 43.66% <20.89%> (-0.21%) ⬇️
gui-tests 58.34% <38.05%> (+0.19%) ⬆️
performance-and-unit-tests ?
test 45.35% <20.89%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/ert/gui/experiments/ensemble_smoother_panel.py 98.71% <100.00%> (ø)
src/ert/gui/experiments/experiment_panel.py 88.94% <ø> (-6.25%) ⬇️
src/ert/gui/experiments/manual_update_panel.py 96.49% <100.00%> (+0.22%) ⬆️
...ui/experiments/multiple_data_assimilation_panel.py 91.56% <100.00%> (+0.21%) ⬆️
src/ert/run_models/model_factory.py 84.61% <ø> (-13.08%) ⬇️
src/ert/config/analysis_module.py 97.22% <0.00%> (-2.78%) ⬇️
src/ert/run_models/run_model_configs.py 94.70% <75.00%> (-5.30%) ⬇️
src/ert/gui/ertwidgets/analysismoduleedit.py 43.28% <26.92%> (-56.72%) ⬇️
...ert/gui/ertwidgets/analysismodulevariablespanel.py 24.05% <24.56%> (-75.95%) ⬇️

... and 271 files with indirect coverage changes

@codspeed-hq

codspeed-hq Bot commented Sep 2, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 36 untouched benchmarks


Comparing frode-aarstad:update-edit-settings-with-update-strategies (2988dcd) with main (03e5ba6)

Open in CodSpeed

@frode-aarstad
frode-aarstad force-pushed the update-edit-settings-with-update-strategies branch from fe3640e to 1145f31 Compare September 3, 2026 07:51
@frode-aarstad
frode-aarstad marked this pull request as ready for review September 3, 2026 08:19
@frode-aarstad
frode-aarstad requested a lite review from Copilot September 3, 2026 08:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

There are confirmed correctness/test-coverage issues (including a potential crash/validation error in the update-settings dialog and ineffective strategy-selection assertions due to missing combobox objectNames).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Implements GUI support for overriding per-parameter-type update strategies (FIELD/SURFACE/GEN_KW) via a new “Update settings” dialog, and refactors the analysis module variable editing widgets/panels and related tests accordingly.

Changes:

  • Refactor AnalysisModuleEdit / AnalysisModuleVariablesPanel to support selecting localization/update strategy per parameter type, and propagate selections back into parameter configs.
  • Update experiment panels (Ensemble smoother / Manual update / Multiple data assimilation) to pass parameter configuration into the editor and rename UI label/title to “Update settings”.
  • Rework/relocate tests: add new unit tests for the new widgets and correlation-threshold logic; remove/adjust older UI tests that no longer match the refactor.
File summaries
File Description
tests/ert/unit_tests/gui/run_analysis/test_analysispanel.py Removes old unit tests for the previous panel API/behavior.
tests/ert/unit_tests/gui/experiments/test_multiple_data_assimilation_panel.py Updates dialog title expectation and removes an old threshold regression UI test.
tests/ert/unit_tests/gui/experiments/test_manual_update.py Updates panel construction to include parameter_configuration.
tests/ert/unit_tests/gui/ertwidgets/test_analysispanel.py Adds unit tests for the refactored variables panel (numeric values + strategy selections).
tests/ert/unit_tests/gui/ertwidgets/test_analysismoduleedit.py Adds unit tests for dialog open/save/cancel behavior in AnalysisModuleEdit.
tests/ert/unit_tests/config/test_analysis_module.py Adds tests for correlation-threshold default/custom behavior.
tests/ert/ui_tests/gui/test_main_window.py Removes a UI test that no longer matches the dialog/widget structure.
src/ert/gui/experiments/multiple_data_assimilation_panel.py Wires parameter configuration into AnalysisModuleEdit and renames label to “Update settings”.
src/ert/gui/experiments/manual_update_panel.py Wires parameter configuration into AnalysisModuleEdit, updates label, and updates ensemble-size handling.
src/ert/gui/experiments/experiment_panel.py Passes parameter configuration into ManualUpdatePanel.
src/ert/gui/experiments/ensemble_smoother_panel.py Wires parameter configuration into AnalysisModuleEdit and renames label to “Update settings”.
src/ert/gui/ertwidgets/analysismodulevariablespanel.py Replaces old “bind directly to analysis module” approach with explicit state + per-type strategy comboboxes.
src/ert/gui/ertwidgets/analysismoduleedit.py Replaces old closable dialog with custom modal dialog supporting per-type strategies and Save/Cancel semantics.
Review details

Suppressed comments (2)

src/ert/gui/ertwidgets/analysismodulevariablespanel.py:72

  • The QComboBox for FIELD doesn't set an objectName, but tests (and potentially other code) rely on identifying comboboxes by parameter type. Without an objectName, those assertions won't run and the widget becomes harder to query.
        field_combobox = QComboBox(self)
        field_combobox.setModel(_LocalizationTypeModel())
        field_combobox.setCurrentIndex(
            self._find_correct_index(field_combobox, "FIELD")
        )

src/ert/gui/ertwidgets/analysismodulevariablespanel.py:85

  • The QComboBox for SURFACE doesn't set an objectName, but tests (and potentially other code) rely on identifying comboboxes by parameter type. Without an objectName, those assertions won't run and the widget becomes harder to query.
        surface_combobox = QComboBox(self)
        surface_combobox.setModel(_LocalizationTypeModel())
        surface_combobox.setCurrentIndex(
            self._find_correct_index(surface_combobox, "SURFACE")
        )
  • Files reviewed: 13/13 changed files
  • Comments generated: 6
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/ert/gui/ertwidgets/analysismoduleedit.py
Comment thread tests/ert/unit_tests/config/test_analysis_module.py
Comment thread src/ert/gui/ertwidgets/analysismodulevariablespanel.py
Comment thread src/ert/gui/ertwidgets/analysismodulevariablespanel.py
Comment thread src/ert/gui/ertwidgets/analysismodulevariablespanel.py Outdated
Comment thread src/ert/gui/experiments/manual_update_panel.py
@frode-aarstad
frode-aarstad force-pushed the update-edit-settings-with-update-strategies branch from 586d318 to 2988dcd Compare September 3, 2026 09:41
@frode-aarstad frode-aarstad changed the title First draft Add controls for configuring localization update strategies Sep 4, 2026
Comment thread src/ert/gui/ertwidgets/analysismoduleedit.py
@erlenlh

erlenlh commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude flagged that Manual Update and ES_MDA restart ignores dialog selection. It uses the stored config/from prior.
Atleast for restart it might not be an issue though, is it decided that when restarting we want to keep all settings the same as the original run?

@frode-aarstad
frode-aarstad force-pushed the update-edit-settings-with-update-strategies branch from 587feb5 to 8cdb41a Compare September 7, 2026 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update analysis config panel with update strategies

5 participants