Remove enkf_obs instance from local_obsdata #3046

joakim-hove · 2022-03-08T06:38:20Z

Add test to LocalObsData. Followup of this comment from #2966

This PR contains some test-code which should be in place before #2966 is merged.

NB: The first commit is a change of behavior (it is also in #2966, but pulled out here to make it more explicit). The situation is:

The C / C++ implementation of local_obs_data does not have any knowledge of the true set of observations in an enkf_obs instance; i.e. you can in principle add a local observation with an unknown key. This is probably a user error - which will go undetected, but not fatal as such - the unknown key will just be dangling and not used for anything.
In the Python implementation LocalObsData we have attached a reference to the enkf_obs instance while constructing the LocalObsData - the purpose of this has been to be able to input check the keys provided by user.

When switching to pybind we do not any longer have separate C and Python implementations - it is all in C++; it is not therefor natural[1] to pimp the python class with respect to the C++ implementation, I therefor suggest to remove this pimping here as a preparation for merging #2966.

[1]: Yes - it would of course still be possible; but in my opinion not worth it. Some of the user control will be recovered when the localisation is completed with #2981. Furthermore it should be noted that the local_config object is decorated with observations, ensemble configuration and grid in Python in the same as described here.

codecov-commenter · 2022-03-08T06:58:54Z

Codecov Report

Merging #3046 (23f4823) into main (ce19612) will increase coverage by 0.12%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##             main    #3046      +/-   ##
==========================================
+ Coverage   65.37%   65.49%   +0.12%     
==========================================
  Files         641      641              
  Lines       50733    50623     -110     
  Branches     4462     4440      -22     
==========================================
- Hits        33167    33158       -9     
+ Misses      16066    15971      -95     
+ Partials     1500     1494       -6

Impacted Files	Coverage Δ
libres/lib/enkf/local_obsdata.cpp	`40.50% <0.00%> (-1.60%)`	⬇️
res/enkf/enkf_main.py	`83.92% <100.00%> (ø)`
res/enkf/local_config.py	`88.88% <100.00%> (-0.45%)`	⬇️
res/enkf/local_obsdata.py	`78.57% <100.00%> (+18.31%)`	⬆️
libres/lib/enkf/local_updatestep.cpp	`75.86% <0.00%> (-6.90%)`	⬇️
libres/lib/analysis/analysis_module.cpp	`24.71% <0.00%> (-1.15%)`	⬇️
libres/lib/enkf/analysis_config.cpp	`48.82% <0.00%> (-0.84%)`	⬇️
libres/lib/enkf/obs_vector.cpp	`38.07% <0.00%> (ø)`
libres/lib/include/ert/analysis/update.hpp	`0.00% <0.00%> (ø)`
libres/lib/res_util/block_fs.cpp	`53.82% <0.00%> (+0.29%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce19612...23f4823. Read the comment docs.

joakim-hove · 2022-03-08T09:00:24Z

Cool if someone can take a peek on the failing Equinor tests.

joakim-hove · 2022-03-08T09:10:59Z

test ert please

oyvindeide

The C / C++ implementation of local_obs_data does not have any knowledge of the true set of observations in an enkf_obs instance; i.e. you can in principle add a local observation with an unknown key. This is probably a user error - which will go undetected, but not fatal as such - the unknown key will just be dangling and not used for anything.

Should be fine, we could do some validation up front, and probably make it fail if unknown keys are included when we run.

In the Python implementation LocalObsData we have attached a reference to the enkf_obs instance while constructing the LocalObsData - the purpose of this has been to be able to input check the keys provided by user.

Agree that this is not the best way to do it, did you check if the existing functionality is used in semeio? Not a problem if it is, just need to update after this is merged.

The PR in itself looks good, some comments regarding the tests.

tests/libres_tests/res/enkf/test_local_obs_data.py

oyvindeide · 2022-03-08T10:59:19Z

tests/libres_tests/res/enkf/test_local_obs_data.py

+from res.enkf.active_list import ActiveList
+
+
+class LocalObsDataTest(ResTest):


Dont use ResTest unless you need any of those features, here unittest.TestCase will do the same job. Personally I would rewrite it to pytest, but not required.

tests/libres_tests/res/enkf/test_local_obs_data.py

res/enkf/local_config.py

joakim-hove · 2022-03-08T12:19:29Z

did you check if the existing functionality is used in semeio?

No I have not checked that.

joakim-hove · 2022-03-08T12:30:04Z

some comments regarding the tests.

Ok - I have applied your patch (thank you), and addressed the other comments.

oyvindeide

LGTM! Though could you also add a summary of this:

The C / C++ implementation of local_obs_data does not have any knowledge of the true set of observations in an enkf_obs instance; i.e. you can in principle add a local observation with an unknown key. This is probably a user error - which will go undetected, but not fatal as such - the unknown key will just be dangling and not used for anything.
In the Python implementation LocalObsData we have attached a reference to the enkf_obs instance while constructing the LocalObsData - the purpose of this has been to be able to input check the keys provided by user.

in the commit body? Would be good to have for posterity.

The C implementation of local_obsdata has no knowledge of the complete set of observation keys; it is therefor quite simple to create a local observation node with an invalid observation key. The observation node corresponding to the invalid node will be dangling - no real harm. In Python we previously we attached a enkf_obs instance to the local_obs instance, in order to be able to validate keys provided by the user. With this PR that 'pimping' of the Python class is removed. This is in preparation to upcoming C++ / pybing refactoring, where there is only the C++ class and not a separate wrapper class in Python.

joakim-hove · 2022-03-08T15:53:05Z

Though could you also add a summary of this ....

done!

joakim-hove · 2022-03-08T18:07:30Z

test required please

joakim-hove marked this pull request as draft March 8, 2022 06:38

joakim-hove mentioned this pull request Mar 8, 2022

Implement local_obsdata as C++ #2966

Merged

joakim-hove marked this pull request as ready for review March 8, 2022 07:46

joakim-hove requested a review from oyvindeide March 8, 2022 07:58

joakim-hove force-pushed the add-obsdata-test branch 2 times, most recently from aca0f16 to 0815af5 Compare March 8, 2022 08:08

joakim-hove force-pushed the add-obsdata-test branch 2 times, most recently from 9db0864 to fce4e73 Compare March 8, 2022 11:57

oyvindeide reviewed Mar 8, 2022

View reviewed changes

joakim-hove force-pushed the add-obsdata-test branch from c8885c8 to ea661be Compare March 8, 2022 12:29

joakim-hove force-pushed the add-obsdata-test branch from ea661be to 23f4823 Compare March 8, 2022 13:08

oyvindeide approved these changes Mar 8, 2022

View reviewed changes

oyvindeide added the release-notes:breaking-change Automatically categorise as breaking change in release notes label Mar 8, 2022

joakim-hove force-pushed the add-obsdata-test branch from 23f4823 to 99305d3 Compare March 8, 2022 15:47

joakim-hove added 2 commits March 8, 2022 16:47

Add testing of local obs data

1be4566

joakim-hove force-pushed the add-obsdata-test branch from 99305d3 to 1be4566 Compare March 8, 2022 15:52

joakim-hove enabled auto-merge (rebase) March 8, 2022 15:53

joakim-hove merged commit 1ff1c5a into equinor:main Mar 8, 2022

joakim-hove deleted the add-obsdata-test branch March 8, 2022 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove enkf_obs instance from local_obsdata #3046

Remove enkf_obs instance from local_obsdata #3046

joakim-hove commented Mar 8, 2022 •

edited

codecov-commenter commented Mar 8, 2022 •

edited

joakim-hove commented Mar 8, 2022

joakim-hove commented Mar 8, 2022

oyvindeide left a comment

oyvindeide Mar 8, 2022

joakim-hove commented Mar 8, 2022

joakim-hove commented Mar 8, 2022

oyvindeide left a comment

joakim-hove commented Mar 8, 2022

joakim-hove commented Mar 8, 2022

		from res.enkf.active_list import ActiveList


		class LocalObsDataTest(ResTest):

Remove enkf_obs instance from local_obsdata #3046

Remove enkf_obs instance from local_obsdata #3046

Conversation

joakim-hove commented Mar 8, 2022 • edited

codecov-commenter commented Mar 8, 2022 • edited

Codecov Report

joakim-hove commented Mar 8, 2022

joakim-hove commented Mar 8, 2022

oyvindeide left a comment

Choose a reason for hiding this comment

oyvindeide Mar 8, 2022

Choose a reason for hiding this comment

joakim-hove commented Mar 8, 2022

joakim-hove commented Mar 8, 2022

oyvindeide left a comment

Choose a reason for hiding this comment

joakim-hove commented Mar 8, 2022

joakim-hove commented Mar 8, 2022

joakim-hove commented Mar 8, 2022 •

edited

codecov-commenter commented Mar 8, 2022 •

edited