Implement ActiveList as C++ and use Pybind

[joakim-hove]

This PR will implement the active_list structure as a prope, C++ class ActiveList and wrap that with pybind. This is part of #2906 (which was a bit much to chew), will continue along the same lines to wrap the classes in the local_config hierarchy.

I understand that there is a desire to replace the entire active_list structure with a std::vector<int>, irrespective of the final decision on that question I hope the current PR (and eventually the rest of #2906) can be merged in something close to it's current state, these changes are needed in order to adress the adaptive localization stuff.

[joakim-hove]

test ert please

[oyvindeide]

Thanks for the PR! I think the changes here are good, but was also wondering if we might be able to do away with ActiveList all together? Or perhaps reduce the usage?

[joakim-hove]

[...] but was also wondering if we might be able to do away with ActiveList all together? Or perhaps reduce the usage?

I understand that the ActiveList class is deemed as an unnecessary complication when e.g. std::vector<int> would do the job. Personally I think the (limited) functionality provided is so core to everything related to local analysis that I would suggest keeping it. But irrespective of the final decision whether to keep it or remove it I would really appreciate if the ongoing refactoring[1] could be completed with the ActiveList intact - in order to (try to) limit the amount of change in this refactoring.

[1]: To implement everything related to local updates with proper C++ and pybind. The full PR was #2906 - but this was to unwieldy and the current PR is a first step in split series.

[pinkwah]

I can agree that it is a benefit to go with pybind11 to make refactoring other parts of the codebase simpler. Have you considered making ActiveList not be a C pointer, since this PR is already touching large swaths of the codebase?

[joakim-hove]

Have you considered making ActiveList not be a C pointer, since this PR is already touching large swaths of the codebase?

Yes. In the first pass I have let C pointers remain C pointers in order to keep the amount of change (somewhat) down - but it is fully my intention to get rid of the C pointers whenever possible.

[oyvindeide]

This looks like a good change 👍 But some code seems to be moved around without being changed? Could you split that into a seperate commit if it needs to move? Would make it easier to review.

[ManInFez]

Failed on jenkins when testing with "equinor data":

=================================== FAILURES ===================================
_____________________ PlotDataTest.test_plot_block_data_fs _____________________

self = <test_plot_data.PlotDataTest testMethod=test_plot_block_data_fs>

   ​def test_plot_block_data_fs(self):
       ​with ErtTestContext("plot_block_data_test", self.config_file) as test_context:
           ​ert = test_context.getErt()
   ​
>           self.checkBlockData(ert, "RFT2", 50)

../../tests/libres_tests/res/enkf/plot/test_plot_data.py:94: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../tests/libres_tests/res/enkf/plot/test_plot_data.py:62: in checkBlockData
   ​self.assertEqual(ert.getEnsembleSize(), len(plot_block_data))
E   AssertionError: 25 != 22

Suspecting flakyness. Can be restarted manually with "test required please!"

[codecov-commenter]

Codecov Report

Merging #2958 (1f6a7b3) into main (1a45f9e) will decrease coverage by 0.13%.
The diff coverage is 27.08%.

@@            Coverage Diff             @@
##             main    #2958      +/-   ##
==========================================
- Coverage   65.82%   65.69%   -0.14%     
==========================================
  Files         639      640       +1     
  Lines       51150    51135      -15     
  Branches     4436     4450      +14     
==========================================
- Hits        33671    33594      -77     
- Misses      15841    15908      +67     
+ Partials     1638     1633       -5     

Impacted Files	Coverage Δ
libres/lib/analysis/update.cpp	29.23% <0.00%> (ø)
libres/lib/enkf/enkf_node.cpp	52.64% <ø> (ø)
libres/lib/enkf/enkf_obs.cpp	68.35% <0.00%> (+0.88%)	⬆️
libres/lib/enkf/field.cpp	0.70% <ø> (ø)
libres/lib/enkf/gen_data.cpp	55.20% <ø> (ø)
libres/lib/enkf/gen_kw.cpp	59.61% <ø> (ø)
libres/lib/enkf/local_ministep.cpp	31.03% <0.00%> (-2.30%)	⬇️
libres/lib/enkf/summary.cpp	32.23% <ø> (ø)
libres/lib/enkf/surface.cpp	2.63% <ø> (ø)
res/enkf/enkf_obs.py	77.77% <ø> (+0.44%)	⬆️
... and 30 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 1a45f9e...1f6a7b3. Read the comment docs.

[joakim-hove]

Suspecting flakyness. Can be restarted manually with "xxxxx"

Thank you! Seems your comment actually provoked a manual restart - and now it is green 🎉

Now merge!

[ManInFez]

Please review the commit history before merging :)

[joakim-hove]

Please review the commit history before merging :)

As it it is now I would say the commits are in atomic units - so the alternatives I see are:

1. Keep as as is (I can optionally try a git dance to apply the formatting at the right commit).
2. Squash everything.

I'll do as told.

[oyvindeide]

Thanks for splitting it up, have some comments and questions.

[ManInFez]

I would squash everything, but keep the commit messages ( except for the formatting and remove print messages)

[oyvindeide]

I would squash everything, but keep the commit messages ( except for the formatting and remove print messages)

I tend to disagree, as I liked it split into atomic units, but will leave the choice up to you.

[oyvindeide]

And just a question, I ran this against the semeio tests, as we are using active lists there, and failed on: https://github.com/equinor/semeio/blob/24bc00bb1755ab420bc4b8e1f1be57c7f4096086/semeio/workflows/correlated_observations_scaling/obs_utils.py#L86 but seems to me that (setParent) should never have been there to begin with as it did not do anything? Not a blocker, will update it once this is merged.

[joakim-hove]

[....] but seems to me that (setParent) should never have been there to begin with as it did not do anything?

I agree - the return value here is a copy - and any keep-alive mechanism is unnecessary.

will update it once this is merged.

Thank you.

[joakim-hove]

OK - I have addressed all comments - still in separate commits, although the "format & debug" commits have been removed.

[oyvindeide]

🚀