Add logging of RFT observations outside grid by HakonSohoel · Pull Request #14322 · equinor/ert

HakonSohoel · 2026-08-31T08:55:17Z

Issue
Resolves #14290

Approach
Add logging for observations deactivated due to beeing outside the grid.
Performed test run on tgx to verify logs arrive as expected

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

codecov-commenter · 2026-08-31T09:08:07Z

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.91%. Comparing base (a78984b) to head (c0be148).
⚠️ Report is 31 commits behind head on main.

Files with missing lines	Patch %	Lines
src/ert/run_models/update_run_model.py	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14322      +/-   ##
==========================================
- Coverage   91.92%   91.91%   -0.01%     
==========================================
  Files         484      485       +1     
  Lines       33662    33671       +9     
==========================================
+ Hits        30944    30950       +6     
- Misses       2718     2721       +3

Flag	Coverage Δ
cli-tests	`36.55% <70.58%> (+<0.01%)`	⬆️
fuzz	`44.38% <70.58%> (+<0.01%)`	⬆️
gui-tests	`58.62% <70.58%> (-0.06%)`	⬇️
performance-and-unit-tests	`80.94% <94.11%> (+0.04%)`	⬆️
test	`45.91% <41.17%> (-0.08%)`	⬇️

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

Files with missing lines	Coverage Δ
src/ert/config/observation_quality_control.py	`100.00% <100.00%> (ø)`
src/ert/run_models/update_run_model.py	`96.96% <93.75%> (-1.07%)`	⬇️

... and 29 files with indirect coverage changes

codspeed-hq · 2026-08-31T09:49:25Z

Merging this PR will not alter performance

✅ 36 untouched benchmarks

_{Comparing HakonSohoel:log-occurrences-of-rft-obs-not-in-grid (c0be148) with main (bf96534)}

achaikou

Mainly wonder if the core of the update code is the right place to put these logs to 🤔

achaikou · 2026-09-02T16:50:13Z

        )

-        smoother_update(
+        smoother_snapshot = smoother_update(


If we are doing it during update, for ES-MDA it would be logged 3 times (though most likely error will be just in the first iteration?). It won't be logged for the final ensemble though, if it suddenly pops up.

This made me wonder what other options we have.

I think RFTConfig's obtain_location_metadata (or _map_locations_to_cells) is a more natural place for this log, even if there potentially would be more logged lines due to each realization being logged separately.
After all, this is where problem first shows itself. So it should be simple to detect and log and it is guaranteed not to be overshadowed by some later code.

What are your thoughts on it?

My reasoning was that we are mainly interested to know to what extent the issue of observations not in the grid affects the update step. Since we in general need to keep the total amount of logs down, to not overflow the logging system, I decided to log once per update step. I also considered logging only once per experiment run, but that would spread the logging code across multiple modules, so once per update seemed like the best trade-of to keep the log count down, while keeping the logging code well contained.

Since the grid can differ between every realization the same observation can be part of the grid in some realizations and not part of the grid in other realizations. Within an iteration if an observations is not part of the grid for any of the realizations that observation will be removed from the update calculation entirely. So it does not really matter if this happens for one, some or many realizations. That's why I only log how many rft observations where deactivated out of the total amount of rft observations per iteration/update-step.

I agree that it would be natural to add the logging somewhere within RFTConfig, but that would make it log per realization which I would like to avoid.

I think observation will be deactivated regardless of us approximating response or not approximating it. It gets deactivated just by mertis of being outside of the grid.
So if I think about it, I am not even sure now if approximating responses for observations / logging how many observations there are outside of the grid is of any use 😅

Alright, I buy the "keeping logs down" reasoning 👍

I guess my main dislike is that this file is still kind of pure, with no pollution from update not-related stuff.
So maybe it makes sense to move the code somewhere closer to what it uses? Like here we rely on having result from get_observations_and_responses, missing_realizations and status columns.
Then better place seems to be here or somewhere nearby
https://github.com/equinor/ert/blob/main/src/ert/analysis/_update_commons.py#L272

But if you prefer update_run_model (or it is easier to mock, for example), then I am fine with it too 🙂

I'll have a look, to see if I can move it. You're correct that observations are deactivated when they are outside the grid currently. As it is now, the approximation option only helps to keep observations active when the response is missing for other reasons (e.g. inactive grid cell). So the aim of logging this particular case is to see if we should change the current behavior, so that responses are approximated for observations outside the grid, and thus allowing the observation to stay active. This would also require us to allow RFT responses and observations to be matched on utm coordinates when the "well_connection_cell" is None.

Do you know why we decided to deactivate observations that are outside of the grid in the first place?

If it was only because there was no matching response, then great. 👍
But if it was to assure that observation actually falls into the field/grid, then I have questions 😄

To my understanding it is just because there will be no matching response. Since grid gaps are just a mathematical artifact due to the way cornerpoint grids define cell surfaces, I see no reason to deactivate such observations for any other reason. Note, if we go down the path of attempting to approximate responses and match against observations outside the grid we would probably want to have some kind of mecanism to disgard observations that are completely outside the grid (.i.e not just located in a gap).

Got you, makes sense!
Thanks for explanations! 🌷

achaikou · 2026-09-02T16:58:49Z

        )
+        self._log_rft_observations_outside_grid(prior, smoother_snapshot)
+
+    def _log_rft_observations_outside_grid(


Might be useful to add some information to the docs about why the function is here and when it can be removed 🪹

Agree, I'll add that

achaikou · 2026-09-02T17:03:38Z

+            {
+                "status": ObservationStatus.MISSING_RESPONSE,
+                "missing_realizations": (
+                    f"0: {RFT_LOCATION_NOT_IN_GRID_ERROR} 10.0, 11.0, 12.0"


Might make sense to add this message not to 1, but to 2 realizations to test that we do not count the same observation twice.

Agree, I'll improve the test

Add logging of RFT observations outside grid

c0be148

HakonSohoel added the release-notes:logging PR which only changes logging. label Aug 31, 2026

achaikou reviewed Sep 2, 2026

View reviewed changes

Uh oh!

Conversation

HakonSohoel commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

When applicable

Uh oh!

codecov-commenter commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codspeed-hq Bot commented Aug 31, 2026

Merging this PR will not alter performance

Uh oh!

achaikou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HakonSohoel commented Aug 31, 2026 •

edited

Loading

codecov-commenter commented Aug 31, 2026 •

edited

Loading