Use the same enum for inversion type for both std_enkf and ies_enkf #2469

joakim-hove · 2021-11-30T12:48:57Z

Part of: #2412

With this PR some of the configuration for the std_enkf and ies_enkf modules are harmonized.

joakim-hove · 2021-11-30T14:39:23Z

test this please

joakim-hove · 2021-12-01T16:32:52Z

test this please

joakim-hove · 2021-12-07T09:24:56Z

test ert please

codecov-commenter · 2021-12-07T09:45:57Z

Codecov Report

Merging #2469 (929f896) into main (a409daf) will decrease coverage by 3.90%.
The diff coverage is 76.78%.

❗ Current head 929f896 differs from pull request most recent head 9550a25. Consider uploading reports for the commit 9550a25 to get more accurate results

@@            Coverage Diff             @@
##             main    #2469      +/-   ##
==========================================
- Coverage   64.42%   60.51%   -3.91%     
==========================================
  Files         645      335     -310     
  Lines       54302    37258   -17044     
  Branches     4537     4545       +8     
==========================================
- Hits        34983    22548   -12435     
+ Misses      17938    13325    -4613     
- Partials     1381     1385       +4

Impacted Files	Coverage Δ
libres/lib/enkf/local_config.cpp	`56.52% <ø> (ø)`
res/enkf/local_config.py	`86.31% <ø> (ø)`
libres/lib/analysis/std_enkf.cpp	`59.23% <76.78%> (+18.21%)`	⬆️
res/job_queue/queue_differ.py	`71.05% <0.00%> (-21.06%)`	⬇️
res/job_queue/queue.py	`69.40% <0.00%> (-12.83%)`	⬇️
res/enkf/observations/gen_observation.py	`80.00% <0.00%> (-8.75%)`	⬇️
res/enkf/export/gen_kw_collector.py	`94.11% <0.00%> (-5.89%)`	⬇️
res/enkf/key_manager.py	`91.42% <0.00%> (-5.72%)`	⬇️
res/job_queue/forward_model_status.py	`88.75% <0.00%> (-5.00%)`	⬇️
res/enkf/enkf_obs.py	`66.96% <0.00%> (-3.58%)`	⬇️
... and 323 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 a409daf...9550a25. Read the comment docs.

joakim-hove · 2021-12-08T15:41:59Z

test ert please

dafeda · 2021-12-09T07:38:24Z

Note that the job ERT - pytest is not Required, which means that we can merge without it passing.
It can fail due to reasons not related to changes in this PR.

dafeda · 2021-12-09T07:46:19Z

libres/lib/analysis/std_enkf.cpp

@@ -96,10 +98,56 @@ static UTIL_SAFE_CAST_FUNCTION_CONST(std_enkf_data, STD_ENKF_TYPE_ID)
    return data->truncation;
 }

+/*
+  The std_enkf module originally used two boolean flags use_EE and use_GE to


For us newbies - what are EE and GE?

Hehe - I am glad you that ask that question .... frankly don't know fully myself either.

It is about choosing the algorithm for (lowrank)inversion of the covariance matrix - which in general do not have. The use_EE flag is whether we should use the approximation C = EE^T. If the use_EE flag is false the (pseudo)inversion is done using the S matrix - without ever constructing or using E.

The difference between use_GE is a formulation where E is used - but not the covariance estimator EE^T.

Would have been healthy to go properly through this again.

The purpose of this PR is alignment and code reuse between the old std_enkf implementation and the newer ies_enkf.

That makes a little bit of sense, but I am not yet that comfortable with the math. Thanks!

I think what we want is to document the different types in ies_inversion_type and link them to literature.

I have created an issue that I've called Questions for Geir Evensen and have now added this question as well.

dafeda · 2021-12-09T08:49:53Z

libres/lib/analysis/std_enkf.cpp

@@ -75,8 +76,9 @@ struct std_enkf_data_struct {
    double truncation; // Controlled by config key: ENKF_TRUNCATION_KEY
    int subspace_dimension; // Controlled by config key: ENKF_NCOMP_KEY (-1: use Truncation instead)
    long option_flags;
-    bool use_EE;
-    bool use_GE;
+    bool __use_EE;


Perhaps add a comment to here that states that both __use_EE and __use_GE are deprecated and shall be removed.
I don't have a clear overview as to what has to be done in order for us to be able to remove them.
Is this something that can be done as part of the ies-refactoring you are doing now, or do we have to make bigger changes?

Perhaps add a comment to here that states that both __use_EE and __use_GE are deprecated and shall be removed.

I will.

I don't have a clear overview as to what has to be done in order for us to be able to remove them.

The only thing which still keeps them around is the users current ability to set them in the config file - i.e. this works:

ANALYSIS_SET STD_ENKF USE_EE True ANALYSIS_SET STD_ENKF USE_GE False

I assume the actual use of such settings is very limited; so after some brief investigation (log peeking?) I guess they can be removed.

Is this something that can be done as part of the ies-refactoring you are doing now

Absolutely - as explained it is more of a political than a technical question.

Good stuff - I've created and issue here: #2534

dafeda · 2021-12-09T08:51:42Z

libres/lib/analysis/std_enkf.cpp

        return name_recognized;
    }
 }

+bool std_enkf_set_string(void *arg, const char *var_name, const char *value) {


Could we use std::string for var_name and value instead of char *?

Ah, I think we can't because it is used as:

.set_string = std_enkf_set_string,

Is that right?

I have not looked into it - but I think that would be a quite large change. The point is that the std_enkf_set_string function is called through a function pointer embedded in: analysis_module - so those changes would ripple a bit here and there. But certainly I agree with the idea, and I am happy to investigate it in a separate PR

dafeda · 2021-12-09T08:56:52Z

libres/tests/analysis/std_enkf.cpp

@@ -0,0 +1,39 @@
+#include <catch2/catch.hpp>
+


Consider changing the name of the test-file to test_std_enkf.cpp.
We are basically following similar naming conventions as used in pytest (not always perfectly though).
Whether or not this is a good idea can be discussed, but it's nice if we can be somewhat consistent I think.

dafeda · 2021-12-09T09:05:17Z

libres/tests/analysis/std_enkf.cpp

+
+#include <ert/analysis/std_enkf.hpp>
+
+TEST_CASE("std_enkf", "[analysis]") {


I think this is a nice candidate to use the GIVEN - WHEN - THEN pattern in catch2 as shown here:

https://github.com/catchorg/Catch2/blob/v2.x/docs/tutorial.md#bdd-style

It's a bit of a pain to get used to at first, but I think it's quite nice.

I think this is a nice candidate to use the GIVEN - WHEN - THEN pattern in catch2 as shown here:

OK - I'll take the pill. Thank you for the pointer.

joakim-hove · 2021-12-09T09:34:16Z

OK @dafeda - thank you for the review; I have tried to address all your comments.

dafeda

LGTM 👍

oyvindeide · 2021-12-10T09:35:00Z

Note that the job ERT - pytest is not Required, which means that we can merge without it passing.
It can fail due to reasons not related to changes in this PR.

That is true, just note that we need to check why it fails before we merge to know that it is not related. You should hopefully have access to check yourself soon.

joakim-hove force-pushed the use-ies-enum branch from 603af0f to e34f7dc Compare December 1, 2021 11:34

joakim-hove closed this Dec 1, 2021

joakim-hove reopened this Dec 1, 2021

joakim-hove force-pushed the use-ies-enum branch from e34f7dc to c9a79f2 Compare December 7, 2021 09:24

joakim-hove force-pushed the use-ies-enum branch from c9a79f2 to 22f864f Compare December 8, 2021 14:48

dafeda reviewed Dec 9, 2021

View reviewed changes

joakim-hove force-pushed the use-ies-enum branch 2 times, most recently from 1af43b5 to d2477a0 Compare December 9, 2021 09:33

dafeda approved these changes Dec 9, 2021

View reviewed changes

joakim-hove force-pushed the use-ies-enum branch from d2477a0 to 17d72ae Compare December 9, 2021 15:09

joakim-hove force-pushed the use-ies-enum branch 2 times, most recently from 4a2f0ef to 929f896 Compare December 14, 2021 08:30

Use the same enum for inversion type for both std_enkf and ies_enkf

9550a25

joakim-hove force-pushed the use-ies-enum branch from 929f896 to 9550a25 Compare December 14, 2021 08:33

joakim-hove merged commit 600567b into equinor:main Dec 14, 2021

joakim-hove deleted the use-ies-enum branch December 14, 2021 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the same enum for inversion type for both std_enkf and ies_enkf #2469

Use the same enum for inversion type for both std_enkf and ies_enkf #2469

joakim-hove commented Nov 30, 2021

joakim-hove commented Nov 30, 2021

joakim-hove commented Dec 1, 2021

joakim-hove commented Dec 7, 2021

codecov-commenter commented Dec 7, 2021 •

edited

joakim-hove commented Dec 8, 2021

dafeda commented Dec 9, 2021

dafeda Dec 9, 2021

joakim-hove Dec 9, 2021

dafeda Dec 9, 2021

dafeda Dec 9, 2021

joakim-hove Dec 9, 2021

dafeda Dec 9, 2021

dafeda Dec 9, 2021

dafeda Dec 9, 2021

joakim-hove Dec 9, 2021

dafeda Dec 9, 2021

joakim-hove Dec 9, 2021

dafeda Dec 9, 2021

joakim-hove Dec 9, 2021

joakim-hove commented Dec 9, 2021

dafeda left a comment

oyvindeide commented Dec 10, 2021


		#include <ert/analysis/std_enkf.hpp>

		TEST_CASE("std_enkf", "[analysis]") {

Use the same enum for inversion type for both std_enkf and ies_enkf #2469

Use the same enum for inversion type for both std_enkf and ies_enkf #2469

Conversation

joakim-hove commented Nov 30, 2021

joakim-hove commented Nov 30, 2021

joakim-hove commented Dec 1, 2021

joakim-hove commented Dec 7, 2021

codecov-commenter commented Dec 7, 2021 • edited

Codecov Report

joakim-hove commented Dec 8, 2021

dafeda commented Dec 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakim-hove commented Dec 9, 2021

dafeda left a comment

Choose a reason for hiding this comment

oyvindeide commented Dec 10, 2021

codecov-commenter commented Dec 7, 2021 •

edited