-
Notifications
You must be signed in to change notification settings - Fork 104
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
Refactor RunDialog to depend less on RunModel #3108
Conversation
344c2ec
to
191d143
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you figure out the segfault?
Yes, I did. Rebasing commits one by one did the trick 😄 |
Codecov Report
@@ Coverage Diff @@
## main #3108 +/- ##
=======================================
Coverage 65.29% 65.30%
=======================================
Files 612 612
Lines 49298 49304 +6
Branches 4445 4445
=======================================
+ Hits 32189 32196 +7
- Misses 15618 15619 +1
+ Partials 1491 1489 -2
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
ert_shared/models/base_run_model.py
Outdated
return True | ||
return False | ||
|
||
def create_mask_from_failed_realizations(self) -> BoolVector: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def create_mask_from_failed_realizations(self) -> BoolVector: | |
def _create_mask_from_failed_realizations(self) -> BoolVector: |
ert_shared/models/base_run_model.py
Outdated
inverted_mask[index] = initial[index] and not successful | ||
return inverted_mask | ||
|
||
def count_successful_realizations(self) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def count_successful_realizations(self) -> int: | |
def _count_successful_realizations(self) -> int: |
@@ -56,6 +64,7 @@ def __init__(self, ert: EnKFMain, queue_config: QueueConfig, phase_count: int = | |||
self._last_run_iteration: int = -1 | |||
self._ert = ert | |||
self.facade = LibresFacade(ert) | |||
self._simulation_arguments = simulation_arguments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe check if members initial_realizations_mask
completed_realizations_mask
can now be made private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is solid stuff. Really good refactor. Had some minor comments to rein in the BRM API.
Tested out all run models in the GUI, seems to work well! Also tried restart on a failed mda run.
👏 👏
Could you shorten the commit message to be < 50 characters 🙂 |
This include updating some of the attributes and functions to become private in RunModel and making simulation arguments implicit attribute to RunModel.
Issue
Resolves #3024
Approach
RunDialog
by migrating simulation running info intoBaseRunModel
- [ ] Makethis might be affected by Experiment server #2132 so left it for nowEvaluatorTracker
as a proxy forBaseRunModel
Pre review checklist
Adding labels helps the maintainers when writing release notes, see sections and the
corresponding labels here: https://github.com/equinor/ert/blob/main/.github/release.yml