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.

Sign up for GitHub

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

Jump to bottom

Enables production data from CSV file and fixes failing probability distribution test #425

Merged
merged 9 commits into from
Aug 6, 2021
Merged

Enables production data from CSV file and fixes failing probability distribution test #425

merged 9 commits into from
Aug 6, 2021

Conversation

edubarrosTNO
Copy link
Contributor

@edubarrosTNO edubarrosTNO commented Jul 22, 2021 •
edited
Loading

Enables production data to be loaded from CSV file and fixes failing probability distribution test

Contributor checklist

  • 🎉 This PR partially addresses Allow for more than one datasource #382.
  • 📜 I have broken down my PR into the following tasks:
    • Modify parser to allow for new optional entry in the configuration file.
    • Create class CSVData as an alternative to FlowData.
    • Replace production data dataframe loaded from reference simulation by data loaded from CSV file.
    • Fix probability distributions test function (deprecated replacement of NaNs by None)
    • Update documentation and CHANGELOG.md.
    • Change config_parser for new position of observation vectors entry in configuration file.
  • 🤖 I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR.
  • 📖 I have considered adding a new entry in CHANGELOG.md.
  • 📚 I have considered updating the documentation.

@edubarrosTNO edubarrosTNO mentioned this pull request Jul 22, 2021
Merged
@edubarrosTNO edubarrosTNO changed the title Enables production data from CSV file Enables production data from CSV file and fixes failing probability distribution test Jul 22, 2021
@edubarrosTNO edubarrosTNO marked this pull request as ready for review July 22, 2021 17:25
olelod
olelod reviewed Jul 30, 2021
View reviewed changes
src/flownet/config_parser/_config_parser.py Show resolved Hide resolved
src/flownet/data/from_csv.py
"""
Function to read production data for all producers and injectors from a CSV file.

Returns:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now the function will return the columns present in the csv file (if e.g. WGPR is not in the csv file it will not be returned with NaN's as I think it will in from_flow.py). I'm not sure if the code needs all vectors, so we may not need NaN's, but there should the docstring should be consistent with the code.

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 don't know if I understand the problem or how to solve it. Are you saying that we should check if all supported summary vectors are present in the CSV file? If so, is there a list of all supported summary vectors anywhere in the code?

@edubarrosTNO edubarrosTNO mentioned this pull request Aug 5, 2021
Merged
@edubarrosTNO
Copy link
Contributor Author

Previous CI test failed because configuration files from https://github.com/equinor/flownet-testdata must be updated (new position of observation vectors entry, PR equinor/flownet-testdata#52). Testdata branch has been changed to verify CI before merging

@edubarrosTNO edubarrosTNO requested a review from olelod August 5, 2021 20:22
@edubarrosTNO
Copy link
Contributor Author

Previous CI test failed because configuration files from https://github.com/equinor/flownet-testdata must be updated (new position of observation vectors entry, PR equinor/flownet-testdata#52). Testdata branch has been changed to verify CI before merging

All CI tests have passed when using the branch of equinor/flownet-testdata#52 - that PR can be merged safely. Next commit of this PR #425 reverts CI back to equinor/master branch, see below

olelod
olelod approved these changes Aug 6, 2021
View reviewed changes
Copy link
Collaborator

@olelod olelod left a comment

Choose a reason for hiding this comment

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

LGTM

@olelod olelod merged commit 9ffca96 into equinor:master Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olelod olelod olelod approved these changes

@fahaddilib fahaddilib Awaiting requested review from fahaddilib fahaddilib is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@edubarrosTNO @olelod