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

Raise exception if analysis can not be performed #3302

Merged
merged 1 commit into from
Apr 28, 2022

Conversation

ManInFez
Copy link
Contributor

@ManInFez ManInFez commented Apr 26, 2022

Issue
Resolves #3008

Approach
Short description of the approach

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.

@ManInFez ManInFez force-pushed the raise_exception_on_analysis_failure branch 3 times, most recently from 79d4e83 to 5078dd7 Compare April 26, 2022 09:55
@ManInFez ManInFez self-assigned this Apr 26, 2022
@ManInFez ManInFez force-pushed the raise_exception_on_analysis_failure branch 6 times, most recently from 0d79ab9 to 15f0b62 Compare April 26, 2022 13:41
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2022

Codecov Report

Merging #3302 (469d260) into main (d11cecc) will increase coverage by 0.00%.
The diff coverage is 74.28%.

@@           Coverage Diff           @@
##             main    #3302   +/-   ##
=======================================
  Coverage   64.94%   64.95%           
=======================================
  Files         616      616           
  Lines       48276    48275    -1     
  Branches     4331     4331           
=======================================
  Hits        31355    31355           
+ Misses      15484    15478    -6     
- Partials     1437     1442    +5     
Impacted Files Coverage Δ
ert_shared/models/iterated_ensemble_smoother.py 87.12% <60.00%> (-0.88%) ⬇️
ert_shared/models/multiple_data_assimilation.py 95.48% <60.00%> (-0.73%) ⬇️
res/enkf/es_update.py 97.61% <71.42%> (+2.93%) ⬆️
ert_gui/tools/run_analysis/run_analysis_tool.py 65.57% <75.00%> (+0.11%) ⬆️
ert_shared/models/ensemble_smoother.py 96.10% <100.00%> (+1.36%) ⬆️
res/enkf/__init__.py 100.00% <100.00%> (ø)
libres/lib/res_util/block_fs.cpp 67.07% <0.00%> (-1.54%) ⬇️
res/enkf/enkf_fs.py 87.50% <0.00%> (+3.40%) ⬆️
res/enkf/enkf_obs.py 81.94% <0.00%> (+4.16%) ⬆️

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

@ManInFez ManInFez added the release-notes:breaking-change Automatically categorise as breaking change in release notes label Apr 27, 2022
@ManInFez ManInFez force-pushed the raise_exception_on_analysis_failure branch 5 times, most recently from d18fb36 to dd76829 Compare April 27, 2022 13:50
Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

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

Some small comments, but a solid improvement 👍

tests/libres_tests/res/enkf/test_es_update.py Outdated Show resolved Hide resolved
ert_shared/models/multiple_data_assimilation.py Outdated Show resolved Hide resolved
ert_shared/models/iterated_ensemble_smoother.py Outdated Show resolved Hide resolved
ert_gui/tools/run_analysis/run_analysis_tool.py Outdated Show resolved Hide resolved
@ManInFez ManInFez force-pushed the raise_exception_on_analysis_failure branch from dd76829 to 153639d Compare April 28, 2022 09:56
)
if len(updatestep) > 1 and module.name() == "IES_ENKF":
raise ErtAnalysisError(
"Can not combine IES_ENKF modules with multi step " "updates"
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably OK but weird

@ManInFez ManInFez force-pushed the raise_exception_on_analysis_failure branch 2 times, most recently from 4fa9852 to 23fc546 Compare April 28, 2022 10:39
Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

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

LGTM!

Raising exceptions will terminate the experiment and potentially
the whole application if analysis can not be performed
@ManInFez ManInFez force-pushed the raise_exception_on_analysis_failure branch from 23fc546 to 469d260 Compare April 28, 2022 10:42
@ManInFez ManInFez merged commit bd70cdc into equinor:main Apr 28, 2022
@ManInFez ManInFez deleted the raise_exception_on_analysis_failure branch April 28, 2022 11:25
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.

Improve handling of update step failing when not in a valid state
4 participants