-
Notifications
You must be signed in to change notification settings - Fork 107
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
Remove the possibility for multiple Steps in a Realization, remove Stage #6313
Conversation
eb193e1
to
3c41825
Compare
c0855d5
to
4c726d5
Compare
2c8ddb1
to
7e65644
Compare
This comment was marked as resolved.
This comment was marked as resolved.
faf8b06
to
c66be59
Compare
729e2a8
to
a63584b
Compare
src/ert/gui/model/snapshot.py
Outdated
@@ -121,13 +119,13 @@ def prerender( | |||
metadata[SORTED_REALIZATION_IDS] = sorted(snapshot.reals.keys(), key=int) | |||
metadata[SORTED_JOB_IDS] = defaultdict(dict) |
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.
Here you can do:
metadata[SORTED_JOB_IDS] = defaultdict(lambda: defaultdict(list))
for idx, _ in job_states.items():
real_id, job_id = idx
metadata[SORTED_JOB_IDS][real_id].append(job_id)
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.
With some modifications, done. Nice.
src/ert/gui/model/snapshot.py
Outdated
@@ -487,10 +461,12 @@ def index(self, row: int, column: int, parent: QModelIndex = None) -> QModelInde | |||
if parent is None: | |||
parent = QModelIndex() | |||
if not self.hasIndex(row, column, parent): | |||
# we end here when parent is real_index, that is wrong!!! |
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.
you can remove these comments now :)
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 _shouldFailJob(self, real, step, job): | ||
return (real, 0, step, job) in self.fail_jobs | ||
def _shouldFailJob(self, real, job): | ||
return (real, 0, job) in self.fail_jobs |
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.
What is the second parameter 0
- can we remove it?
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.
Thats probably a step that we should remove. Nice catch!
@@ -194,11 +184,11 @@ def evaluate(self, config): | |||
def start(self): | |||
self._eval_thread.start() | |||
|
|||
def _shouldFailJob(self, real, step, job): | |||
return (real, 0, step, job) in self.fail_jobs | |||
def _shouldFailJob(self, real, job): |
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 function is used only once, so I think it can be inlined instead.
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 self.fail_jobs
list is never updated so the if-sentence using this is probably never hit. This is potentially something unfinished in the test code, and fixing it or removing it should be its own PR.
|
||
def addFailJob(self, real, step, job): | ||
self.fail_jobs.append((real, 0, step, job)) | ||
def addFailJob(self, real, job): |
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.
✂️ - not in use.
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.
One thing that I would include is a small comment (a paragraph perhaps?) explaining what's the purpose of this Step singleton under the realization. |
Step "id" is removed, as there is now only one step (the "zero" step). The step is kept for now as it contains needed information for how to run the realization. This information is later to be merged upwards to the realization itself.
b5ee1f8
to
d787e8b
Compare
Added a sentence in the git commit log. Looking at the source I find it readable due to the dataclass structure. |
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.
Amazing job @berland ! 🚀
Step "id" is removed, as there is now only one step (the "zero" step).
Issue
Partially resolves #6218
Approach
✂️ and removing the Step layer in several tree structures.
Pre review checklist
Ground Rules),
and changes to existing code have good test coverage.
Pre merge checklist