Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Join EnKFMain, _RealEnKFMain, EnkfFsManager and EnkfJobRunner in python #3705

Merged
merged 1 commit into from
Aug 5, 2022

Conversation

oyvindeide
Copy link
Collaborator

@oyvindeide oyvindeide commented Aug 4, 2022

This joins the different classes that all point to the
underlying C++ object enkf_main so there is a 1 to 1
relationship. The reason for doing this is that the split
introduces a lot of complexity, and no real separation of
concerns, as the subclasses often have to call parent().
There was also a lot of additional complexity added on the
python side, for example monkeypatching methods, this has
been simplified, and the overall result is that the representation
is more honest about what is actually going on.

Pre review checklist

  • Added appropriate release note label
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Commit history is consistent and clean, in line with the contribution guidelines.

Adding labels helps the maintainers when writing release notes. This is the list of release note labels.

@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2022

Codecov Report

Merging #3705 (9a5e315) into main (aa0b89b) will decrease coverage by 0.00%.
The diff coverage is 93.39%.

@@            Coverage Diff             @@
##             main    #3705      +/-   ##
==========================================
- Coverage   63.28%   63.27%   -0.01%     
==========================================
  Files         595      594       -1     
  Lines       46198    46095     -103     
  Branches     4156     4156              
==========================================
- Hits        29236    29168      -68     
+ Misses      15667    15632      -35     
  Partials     1295     1295              
Impacted Files Coverage Δ
src/ert_shared/models/base_run_model.py 89.55% <ø> (-0.05%) ⬇️
src/ert_shared/libres_facade.py 91.97% <72.72%> (ø)
src/res/enkf/enkf_main.py 95.62% <93.52%> (+6.58%) ⬆️
src/ert_shared/models/ensemble_experiment.py 95.52% <100.00%> (ø)
src/ert_shared/models/ensemble_smoother.py 95.83% <100.00%> (ø)
...rc/ert_shared/models/iterated_ensemble_smoother.py 88.77% <100.00%> (ø)
...rc/ert_shared/models/multiple_data_assimilation.py 95.31% <100.00%> (ø)
src/res/enkf/enkf_fs_manager.py 81.57% <100.00%> (-9.03%) ⬇️
src/res/simulator/simulation_context.py 89.01% <100.00%> (-0.12%) ⬇️
src/ert/gui/ertwidgets/__init__.py 77.27% <0.00%> (+2.27%) ⬆️
... and 1 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@oyvindeide oyvindeide marked this pull request as ready for review August 5, 2022 10:17
Copy link
Contributor

@dafeda dafeda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@oyvindeide oyvindeide added the release-notes:breaking-change Automatically categorise as breaking change in release notes label Aug 5, 2022
@oyvindeide oyvindeide changed the title Rejoin enkf main Join EnKFMain, _RealEnKFMain, EnkfFsManager and EnkfJobRunner in python Aug 5, 2022
@oyvindeide oyvindeide force-pushed the rejoin_enkf_main branch 7 times, most recently from d3d8078 to 18fe4d9 Compare August 5, 2022 13:33
This joins the different classes that all point to the
underlying C++ object enkf_main so there is a 1 to 1
relationship. The reason for doing this is that the split
introduces a lot of complexity, and no real separation of
conserns, as the subclasses often have to call parent().
There was also a lot of additional complexity added on the
python side, for example monkeypatching methods, this has
been simplified, and the overall result is that the representation
is more honest about what is actually going on.
@oyvindeide oyvindeide enabled auto-merge (rebase) August 5, 2022 13:50
@oyvindeide oyvindeide merged commit 1e6034a into equinor:main Aug 5, 2022
@oyvindeide oyvindeide deleted the rejoin_enkf_main branch May 2, 2023 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:breaking-change Automatically categorise as breaking change in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants