Move create runpath and sample parameter logic from C to Python #3467
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3467 +/- ##
==========================================
- Coverage 65.40% 64.50% -0.91%
==========================================
Files 601 601
Lines 47788 47727 -61
Branches 4215 4213 -2
==========================================
- Hits 31257 30786 -471
- Misses 15170 15628 +458
+ Partials 1361 1313 -48
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
oyvindeide
force-pushed
the
move_logic
branch
3 times, most recently
from
394ba9d
to
99a67e7
Compare
oyvindeide
force-pushed
the
move_logic
branch
4 times, most recently
from
9e5506c
to
f0e870f
Compare
This moves more logic into python and since C no longer has responsibility to sample parameters a few tests are moved into python as well.
oyvindeide
changed the title
oyvindeide
added
the
release-notes:maintenance
There was a problem hiding this comment.
Generally a good idea to move as much as possible into python. I had one small suggestion about adding some more tests in c, but other than that it looks good. Perhaps @pinkwah could have a quick look at the pybind magic to make sure its optimal :)
|
|
||
| namespace fs = std::filesystem; | ||
|
|
||
| void create_runpath(enkf_main_type *enkf_main, int iter) { |
There was a problem hiding this comment.
I see that these c tests in general are replaced with python tests because enkf_main_create_run_path and enkf_main_initialize_from_scratch no longer exists. The tests are a bit messy in terms of purpose and are in the "old" style so I think that is ok.
However, this is leaving libres/lib/enkf/gen_data.cpp, libres/lib/enkf/enkf_state.cpp and libres/lib/enkf/enkf_plot_gen_kw.cpp without tests in c. Perhaps some very basic isolated unit-tests in the new c style could be added to make up for the lost c tests? When it comes to memory-debugging etc. its often a lot easier to work with c tests for c code.
There was a problem hiding this comment.
There are some tests for them (in the old style, for example: test_enkf_gen_data_config.cpp, test_enkf_gen_data_config_parse.cpp), adding some new ones is possible but not sure that the effort is worth it.
There was a problem hiding this comment.
I think you are right. Lets not dwell too much on tests for these cases.
Issue
Resolves #3389
Approach
Short description of the approach
Pre review checklist
Adding labels helps the maintainers when writing release notes. This is the list of release note labels.