Merge ForwardModel with ExtJob #6489
Conversation
berland
changed the title
berland
changed the title
berland
force-pushed
the
merge_forwardmodel_extjob
branch
from
73c8903
to
a3c5f75
Compare
The ForwardModel class in ensemble_evaluator is removed, and ExtJob is renamed to ForwardModel. The ForwardModel aka ExtJob is then used directly in the Realization object in ensemble_evaluator.
berland
force-pushed
the
merge_forwardmodel_extjob
branch
from
a3c5f75
to
e2be21f
Compare
berland
added
the
release-notes:maintenance
Codecov Report
@@ Coverage Diff @@
## main #6489 +/- ##
==========================================
- Coverage 83.49% 83.48% -0.01%
==========================================
Files 341 341
Lines 20716 20710 -6
Branches 938 938
==========================================
- Hits 17296 17289 -7
Misses 3130 3130
- Partials 290 291 +1
... and 1 file with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
| @@ -10,7 +10,7 @@ class StrEnum(str, Enum): | |||
| from enum import StrEnum | |||
|
|
|||
|
|
|||
| class ExtJobKeys(StrEnum): | |||
| class ForwardModelKeys(StrEnum): | |||
| NAME = "NAME" | |||
There was a problem hiding this comment.
Would it be more "python-way" to use auto() instead of hardcoding "NAME" and "EXECUTABLE"? That seems like one of the benefits of inheriting from StrEnum
There was a problem hiding this comment.
** I just realized there were way more enums than those two. We should probably replace all the hardcoded values with auto to be DRY "compliant"
There was a problem hiding this comment.
Yes, should be its own issue.
| for forward_model in real.forward_models: | ||
| reals[str(real.iens)].jobs[str(forward_model.id_)] = Job( | ||
| for index, forward_model in enumerate(real.forward_models): | ||
| reals[str(real.iens)].jobs[str(index)] = Job( |
There was a problem hiding this comment.
Could there be a situation where forward_model.id_ would be different than its index in the list?
There was a problem hiding this comment.
Maybe there were situations like that earlier, but I haven't found such situations now. Tests do not uncover any issues with this simplification at least. I think not relying on this integer index is an ert3 pattern.
Issue
Part of #6218
Approach
Short description of the approach
(Screenshot of new behavior in GUI if applicable)
Pre review checklist
Ground Rules),
and changes to existing code have good test coverage.
Pre merge checklist