Skip to content

Log param update strategies - #14320

Open
tbwiik wants to merge 4 commits into
equinor:mainfrom
tbwiik:log-update-strats
Open

Log param update strategies#14320
tbwiik wants to merge 4 commits into
equinor:mainfrom
tbwiik:log-update-strats

Conversation

@tbwiik

@tbwiik tbwiik commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Issue
Resolves #14260

Approach
Adds the following logs (triggered when running poly):

INFO - Update strategy 'GLOBAL' for parameters 'a, b, c' 
  • 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')

@tbwiik tbwiik added the release-notes:logging PR which only changes logging. label Aug 31, 2026
@tbwiik
tbwiik requested a review from xjules August 31, 2026 07:43
@tbwiik

tbwiik commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Based on the sorrounding logs it seems like it would be more correct to change to past tense?

@codecov-commenter

codecov-commenter commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.28571% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.10%. Comparing base (bf96534) to head (38fcb46).
⚠️ Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
src/ert/analysis/_es_update.py 89.28% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14320      +/-   ##
==========================================
- Coverage   91.92%   91.10%   -0.83%     
==========================================
  Files         485      498      +13     
  Lines       33655    36225    +2570     
==========================================
+ Hits        30939    33003    +2064     
- Misses       2716     3222     +506     
Flag Coverage Δ
cli-tests 35.50% <85.71%> (-1.04%) ⬇️
fuzz 43.72% <85.71%> (-0.65%) ⬇️
gui-tests 58.27% <85.71%> (-0.35%) ⬇️
performance-and-unit-tests 81.14% <89.28%> (+0.19%) ⬆️
test 45.27% <14.28%> (-0.65%) ⬇️

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

Files with missing lines Coverage Δ
src/ert/analysis/_es_update.py 94.02% <89.28%> (+0.04%) ⬆️

... and 49 files with indirect coverage changes

@codspeed-hq

codspeed-hq Bot commented Aug 31, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 36 untouched benchmarks


Comparing tbwiik:log-update-strats (38fcb46) with main (23d74ff)

Open in CodSpeed

@tbwiik

tbwiik commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Proposed format instead:

  • group on update strategy type
    Ex: Update strategy "global": <param a>, <param b>

@xjules

xjules commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Proposed format instead:

  • group on update strategy type
    Ex: Update strategy "global": <param a>, <param b>

Yes, this is a good proposal :)

@ertomatic

Copy link
Copy Markdown
Collaborator

Screenshots differ from baselines. A baseline update PR has been prepared: equinor/ert-testdata#110

@tbwiik

tbwiik commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Definitely a false negative from ertomatic. @berland why was this triggered this time?

@ertomatic

Copy link
Copy Markdown
Collaborator

Screenshots differ from baselines. A baseline update PR has been prepared: equinor/ert-testdata#111

@tbwiik

tbwiik commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Please see PR description for finalized format

@tbwiik

tbwiik commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Or maybe it isn't a false negative, because the coordinates are actually changed. However, as I have only done a refactor and added logs this should not have modified that part of the code

@tbwiik

tbwiik commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

False negative from ertomatic. See #13932 (comment)

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

The new logging/refactor introduces misleading warning text and unguarded INFO-level formatting work, and the new user-visible log behavior should be covered by an automated test.

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

Pull request overview

This PR addresses #14260 by ensuring the configured update strategy per parameter group is logged during analysis updates, improving traceability of which localization strategy is applied.

Changes:

  • Refactors build_strategy_map() to centralize strategy construction/selection logic into helper functions.
  • Adds an INFO log summary that groups parameter names by update strategy.
File summaries
File Description
src/ert/analysis/_es_update.py Refactors strategy-map construction and adds grouped INFO logging of update strategies per parameter group.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 3
  • 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/analysis/_es_update.py Outdated
Comment thread src/ert/analysis/_es_update.py
Comment thread src/ert/analysis/_es_update.py
Separate different error cases when resolving strategy

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

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

Exact-type dispatch breaks previously supported parameter-config subclasses, and the new clustering assertion is insufficiently precise.

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

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

if param_cfg.update_strategy is None:
return None

handler = strategy_handlers.get(type(param_cfg))
Comment thread tests/ert/unit_tests/analysis/test_es_update.py Outdated
@tbwiik

tbwiik commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Got some refactor changes from CoPilot which seemed nice. It seems they were less nice, and that behavioral changes were added...
Looking at this Monday - possibly reverting the refactor and only keeping the intended changes.
#scopecreep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes:logging PR which only changes logging.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make sure that the update strategies are logged

5 participants