Skip to content

Add config for update strategy for parameter types#13048

Open
frode-aarstad wants to merge 4 commits intoequinor:mainfrom
frode-aarstad:add-strategy-for-parameter-type
Open

Add config for update strategy for parameter types#13048
frode-aarstad wants to merge 4 commits intoequinor:mainfrom
frode-aarstad:add-strategy-for-parameter-type

Conversation

@frode-aarstad
Copy link
Copy Markdown
Contributor

Issue
Resolves #13027

Approach
Short description of the approach

(Screenshot of new behavior in GUI if applicable)

  • 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 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 Mar 6, 2026
@frode-aarstad frode-aarstad force-pushed the add-strategy-for-parameter-type branch from 077c49b to 1349de4 Compare March 6, 2026 08:41
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.53%. Comparing base (d005912) to head (af6dd86).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13048      +/-   ##
==========================================
- Coverage   90.55%   90.53%   -0.02%     
==========================================
  Files         454      454              
  Lines       31233    31249      +16     
==========================================
+ Hits        28282    28292      +10     
- Misses       2951     2957       +6     
Flag Coverage Δ
cli-tests 37.28% <50.00%> (+<0.01%) ⬆️
gui-tests 67.99% <50.00%> (-0.04%) ⬇️
performance-and-unit-tests 77.12% <100.00%> (-0.01%) ⬇️
test 46.17% <40.00%> (-0.04%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds support in AnalysisConfig for configuring a default update strategy per parameter type via ANALYSIS_SET_VAR PARAMETERS ..., aligning with issue #13027’s requested configuration capability.

Changes:

  • Parse ANALYSIS_SET_VAR PARAMETERS <PARAMETER_TYPE> <STRATEGY> into a new AnalysisConfig.parameter_settings structure.
  • Add validation errors for unknown parameter types and strategy names.
  • Add unit tests covering successful parsing and validation failures.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/ert/config/analysis_config.py Introduces ParameterSettings and parsing/validation of ANALYSIS_SET_VAR PARAMETERS ... into AnalysisConfig.
tests/ert/unit_tests/config/test_analysis_config.py Adds unit tests for the new parameter strategy configuration parsing and validation.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 9, 2026

Merging this PR will not alter performance

✅ 35 untouched benchmarks


Comparing frode-aarstad:add-strategy-for-parameter-type (af6dd86) with main (fd15e01)

Open in CodSpeed

ANALYSIS_SET_VAR PARAMETERS FIELD DISTANCE
ANALYSIS_SET_VAR PARAMETERS GEN_KW ADAPTIVE


Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I need some help filling out the docs here

@xjules xjules added the blocked label Mar 11, 2026
@xjules
Copy link
Copy Markdown
Contributor

xjules commented Mar 11, 2026

We will need to link it to the storage (maybe parameter configs?) and currently we don't have a consensus on whether it should be the full analysis config info (and thus dropping the analysis config entirely) or just to have a name of the UpdateStrategy.
Related issue: #13049

@xjules xjules added the needs-discussion Issues requiring further discussions label Mar 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked needs-discussion Issues requiring further discussions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add configuration for default update strategy per parameter type

4 participants