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

Show restart button when all realisations fail #4113

Merged
merged 1 commit into from
Oct 21, 2022

Conversation

dafeda
Copy link
Contributor

@dafeda dafeda commented Oct 21, 2022

Issue
Resolves #4112

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.

@dafeda dafeda requested a review from berland October 21, 2022 08:01
@dafeda dafeda self-assigned this Oct 21, 2022
@dafeda dafeda added the release-notes:bug-fix Automatically categorise as bug fix in release notes label Oct 21, 2022
self._initial_realizations_mask, self._completed_realizations_mask
)
]
if len(self._completed_realizations_mask) == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to if self._completed_realizations_mask is None instead, and then have this attribute None from object initialization and also set to None instead of [] in the case of an ErtRunError (which I speculate is happening here?)

Copy link
Contributor

Choose a reason for hiding this comment

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

If possible (meaning that the ensemble length is always known) we can also change it to always have the correct length (but it is maybe needed to allow None at init?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we change this to if self._completed_realizations_mask is None instead, and then have this attribute None from object initialization and also set to None instead of [] in the case of an ErtRunError (which I speculate is happening here?)

We would then also have to change the input argument to BaseRunModel to Optional[List[bool]] = None right?
Not sure we gain that much from using None instead of [] here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. A zero-length ensemble is always an error in Ert (?) so [] is "free" to be used equivalent to None. It might be cleaner still to use None as [] could be thought of as ok if one believes zero-length ensembles is a thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I tried running poly.ert with 0 realisations and when trying to run an ensemble experiment get this crash:

ValueError: Wrong range syntax 0--1

😬

Copy link
Contributor

Choose a reason for hiding this comment

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

that is ok!

Copy link
Contributor

@berland berland Oct 21, 2022

Choose a reason for hiding this comment

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

(but pylint frowns upon if len(list) == 0, do if not list instead.)

Copy link
Contributor Author

@dafeda dafeda Oct 21, 2022

Choose a reason for hiding this comment

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

Pylint does not like bare len(list), explicit checks like len(list)==0 are fine according to pep8.
This was considered a bug in pylint:
pylint-dev/pylint#2815

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, but pep8 still says that if not list is good in this situation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with either as long as the bug is fixed.
I've pushed a version that uses if list.

Copy link
Contributor

@berland berland left a comment

Choose a reason for hiding this comment

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

💯

@dafeda dafeda merged commit 0324ea0 into equinor:main Oct 21, 2022
@dafeda dafeda deleted the restart_button branch October 21, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:bug-fix Automatically categorise as bug fix in release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Restart button not showing when all realisations fail
2 participants