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

Add analysis workflow as optional #90

Merged

Conversation

edubarrosTNO
Copy link
Contributor

@edubarrosTNO edubarrosTNO commented Jul 3, 2020

Closes #61.

With this change, the analysis workflow is not added to the ERT run if ert.analysis is not present in the FlowNet config file.
This PR also includes the changes in the other files to accompany the fix in #81 (I had forgotten to commit those).

src/flownet/templates/ahm_config.ert.jinja2 Outdated Show resolved Hide resolved
src/flownet/templates/ahm_config.ert.jinja2 Outdated Show resolved Hide resolved
@edubarrosTNO edubarrosTNO requested review from wouterjdb and olwijn July 6, 2020 12:42
@@ -1,2 +1 @@
SAVE_ITERATION_ANALYTICS_WORKFLOW_JOB <RUN_PATH> <ECL_BASE> <YAML_OBS> <ANALYSIS_START> <ANALYSIS_END> <ANALYSIS_QUANTITY> <ANALYSIS_METRIC> <ANALYSIS_OUTFILE>

SAVE_ITERATION_ANALYTICS_WORKFLOW_JOB <CONFIG_FILE> <RUN_PATH> <ECL_BASE>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
SAVE_ITERATION_ANALYTICS_WORKFLOW_JOB <CONFIG_FILE> <RUN_PATH> <ECL_BASE>
SAVE_ITERATION_ANALYTICS_WORKFLOW_JOB <CONFIG_FILE> <RUN_PATH> <ECL_BASE>

@wouterjdb
Copy link
Collaborator

Where in this PR is #61 addressed?

@edubarrosTNO
Copy link
Contributor Author

Two actions were identified in issue #61 :

  1. Change the way data from reference OPM-Flow/Eclipse simulation is loaded. Previously, this data was loaded by a method that read the values from the observation file into a dictionary, causing random behavior because order is not preserved. PR Fixes bug in analysis workflow #81 addressed that: now the data from reference simulation is read directly from the simulation summary file.
  2. Make the analysis workflow optional. This PR Add analysis workflow as optional #90 addresses this: now, if no analysis entry in the FlowNet config file, the post-simulation workflow SAVE_ITERATION_ANALYTICS_WORKFLOW will not be run in ERT. And because it is optional now, this PR also removes the default values of the input to the analysis keyword in the FlowNet config file.

Does this answer your question, @wouterjdb ?

@wouterjdb
Copy link
Collaborator

The following two problems were also mentioned in #61; are those addressed or open issues:

  1. If observations do not exists for all vectors that are requested the algorithm fails
  2. If a well does not run all the way to the end of the simulation NaN values are introduced in the algorithm (and thus it will crash).

@edubarrosTNO
Copy link
Contributor Author

It could be that the new changes fix those, but I didn't address or test them directly. So I would say maybe better to keep them as separate open issues.

We have a well closing in the Brugge example, and I don't observe any NaN. So it could be that your point (2) is covered.

Regarding your point (1), I personally think that it should be left to the user the responsibility to choose appropriately the vectors for the analysis. With the new changes, data from the reference OPM-Flow/Eclipse simulation is fetched from the summary files of the input_case using the flownet.data.from_flow method. So, in principle, we could now run the analysis workflow not only for vectors present in the observation file, but any summary keyword that is reported in both OPM-Flow/Eclipse and FlowNet simulations. Then again, I think it should be responsibility of the user to make sure that the keywords/vectors are reported; we could add a warning in case the keyword doesn't exist though.

Still related to this, currently flownet.data.from_flow only supports well-based keywords/vectors; it could be desirable to extend this to field, region and group keywords/vectors in the future. This would allow us to derive accuracy in terms of other quantities, not only to well information.

@edubarrosTNO
Copy link
Contributor Author

By the way, this PR #90 is a bit on hold now. The latest changes in the master caused the code on my local branch not to work anymore. I might start a new PR with the commits again

@wouterjdb
Copy link
Collaborator

By the way, this PR #90 is a bit on hold now. The latest changes in the master caused the code on my local branch not to work anymore. I might start a new PR with the commits again

Solved the conflict.

@wouterjdb
Copy link
Collaborator

wouterjdb commented Jul 14, 2020

Maybe add the analysis workflow to tests/configs/norne_parameters.yml? Then we can check that it runs without errors in the automated testing runs.

This will now fail because the configuration file is called differently (see requested change below).

NB. Don't forget to pull the changes I made before you continue working.

@wouterjdb wouterjdb added bug Something isn't working enhancement New feature or request labels Jul 14, 2020
@edubarrosTNO edubarrosTNO force-pushed the add-analysis-workflow-as-optional branch from 36da28a to 4941a83 Compare August 14, 2020 09:43
@edubarrosTNO edubarrosTNO requested a review from wouterjdb August 17, 2020 09:58
Copy link
Collaborator

@wouterjdb wouterjdb left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@edubarrosTNO edubarrosTNO merged commit 871d139 into equinor:master Aug 21, 2020
@edubarrosTNO edubarrosTNO deleted the add-analysis-workflow-as-optional branch September 4, 2020 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in flownet_save_iteration_analytics in CI/CD build
3 participants