Skip to content

Include all numeric params enif#13286

Draft
larsevj wants to merge 1 commit intoequinor:mainfrom
larsevj:include_all_numeric_params_enif
Draft

Include all numeric params enif#13286
larsevj wants to merge 1 commit intoequinor:mainfrom
larsevj:include_all_numeric_params_enif

Conversation

@larsevj
Copy link
Copy Markdown
Collaborator

@larsevj larsevj commented Apr 10, 2026

Issue
Resolves #my_issue

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')

@larsevj larsevj force-pushed the include_all_numeric_params_enif branch from e44cb11 to 53560c5 Compare April 10, 2026 10:48
@larsevj larsevj requested a review from Copilot April 10, 2026 10:55
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 10, 2026

Codecov Report

❌ Patch coverage is 82.75862% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.43%. Comparing base (5be6edd) to head (7bffc16).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/ert/analysis/_enif_update.py 82.75% 5 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (5be6edd) and HEAD (7bffc16). Click for more details.

HEAD has 12 uploads less than BASE
Flag BASE (5be6edd) HEAD (7bffc16)
test 5 4
gui-tests 5 4
performance-and-unit-tests 5 0
cli-tests 5 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main   #13286       +/-   ##
===========================================
- Coverage   90.05%   74.43%   -15.62%     
===========================================
  Files         459      459               
  Lines       32087    32107       +20     
===========================================
- Hits        28895    23899     -4996     
- Misses       3192     8208     +5016     
Flag Coverage Δ
cli-tests ?
fuzz 44.17% <3.44%> (-0.03%) ⬇️
gui-tests 66.86% <82.75%> (+<0.01%) ⬆️
performance-and-unit-tests ?
test 45.80% <3.44%> (-0.03%) ⬇️

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

Files with missing lines Coverage Δ
src/ert/run_models/ensemble_information_filter.py 91.30% <ø> (-8.70%) ⬇️
src/ert/run_models/manual_update_enif.py 93.75% <ø> (ø)
src/ert/analysis/_enif_update.py 81.60% <82.75%> (-14.60%) ⬇️

... and 202 files with indirect coverage changes

Copy link
Copy Markdown
Contributor

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

This PR updates the EnIF analysis workflow to build the regression/H-map (and related precision structure) using all numeric parameter groups from the experiment configuration, while still only persisting updates to update=True parameter groups.

Changes:

  • Remove the explicit parameters argument from enif_update / analysis_EnIF and derive parameter groups from the experiment configuration instead.
  • Add _load_numeric_parameters to load and filter parameter groups to numeric-only for EnIF computations.
  • Extend unit/UI/performance tests to cover inclusion of non-updateable numeric parameters in the regression inputs and update the new enif_update call signature.

Reviewed changes

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

Show a summary per file
File Description
src/ert/analysis/_enif_update.py Derives EnIF predictors from numeric parameter groups; updates storage behavior to only write updateable groups; removes parameters arg.
src/ert/run_models/manual_update_enif.py Updates EnIF invocation to match new signature (no parameters=).
src/ert/run_models/ensemble_information_filter.py Updates EnIF invocation to match new signature (no parameters=).
tests/ert/unit_tests/analysis/test_enif_update.py Adds unit coverage for _load_numeric_parameters numeric filtering/casting behavior.
tests/ert/ui_tests/cli/analysis/test_enif_update.py Adds UI-level coverage ensuring non-updateable numeric params contribute to regression but are not modified.
tests/ert/performance_tests/test_obs_and_responses_performance.py Updates benchmark calls to enif_update to match the new signature.
tests/ert/performance_tests/test_memory_usage.py Updates memory test call to enif_update to match the new signature.

Comment thread src/ert/analysis/_enif_update.py Outdated
Comment on lines 57 to 61
def enif_update(
prior_storage: Ensemble,
posterior_storage: Ensemble,
observations: Iterable[str],
parameters: Iterable[str],
random_seed: int,
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

enif_update is exported from ert.analysis, and this change removes the public parameters argument entirely. If external callers relied on filtering parameters explicitly, this becomes a breaking API change; consider keeping parameters as an optional/keyword-only argument (defaulting to "all numeric") with a deprecation path, or ensure the change is documented/released as breaking.

Copilot uses AI. Check for mistakes.
@larsevj larsevj force-pushed the include_all_numeric_params_enif branch from 53560c5 to 91f4a32 Compare April 10, 2026 11:20
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 10, 2026

Merging this PR will not alter performance

✅ 36 untouched benchmarks


Comparing larsevj:include_all_numeric_params_enif (d9f430b) with main (97eaab5)

Open in CodSpeed

@larsevj larsevj force-pushed the include_all_numeric_params_enif branch from 91f4a32 to d9f430b Compare April 24, 2026 12:24
@larsevj larsevj added this to SCOUT Apr 30, 2026
@larsevj larsevj moved this to In Progress in SCOUT Apr 30, 2026
@larsevj larsevj self-assigned this Apr 30, 2026
@larsevj larsevj force-pushed the include_all_numeric_params_enif branch from d9f430b to 7bffc16 Compare April 30, 2026 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants