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 Genertobs #704

Open
wants to merge 281 commits into
base: main
Choose a base branch
from
Open

Add Genertobs #704

wants to merge 281 commits into from

Conversation

daniel-sol
Copy link

@daniel-sol daniel-sol commented Jun 4, 2024

Solving issue #683. Add module for generation of observation data for ert from yaml config file that takes csv, or spreadsheets as input.

Extra feature added: Users can deactivate observations, either in the config file to remove the entire observation group:

- name: Gas-oil-ratio observations
  type: timeseries
  error: 10%  # optional (% indicates relative, otherwise absolute)
  active: false
  observation: ./input/observations/summary_gor.csv

Or directly in the csv file, by adding an active column.
This column is not required, and when not present all observations are included
When present only the entries that do not include a no will be included

value error active
0.25 0.1
0.23 0.09
.. .. ..
.. .. ..
0.32 0.15 no
0.33 0.15 no
.. .. ..

The name active has been by several suggested to be suboptimal, any suggestion for a different name would be appreciated

@daniel-sol daniel-sol requested review from alifbe and rnyb June 4, 2024 12:58
@mferrera
Copy link
Collaborator

mferrera commented Jun 10, 2024

I'm uncertain that the upload workflow should live in this repository. Architecturally it feels more appropriate for that functionality to live in the Sumo world, particularly if it's importing private classes and functions from fmu-sumo-uploader anyway. Additionally, I don't think we want fmu-sumo-uploader@git+https://github.com/equinor/fmu-sumo-uploader (bleeding) as a dependency here.

Have you considered locating the uploader there?

@daniel-sol
Copy link
Author

I'm uncertain that the upload workflow should live in this repository. Architecturally it feels more appropriate for that functionality to live in the Sumo world, particularly if it's importing private classes and functions from fmu-sumo-uploader anyway. Additionally, I don't think we want fmu-sumo-uploader@git+https://github.com/equinor/fmu-sumo-uploader (bleeding) as a dependency here.

Have you considered locating the uploader there?

I think this a fair point, I was just very focused on making it work, therefore I added the uploader as a dependency so that one could test it during development. But I think it makes to generate the include file if the cli script upload_obs_2_sumo (or upload_preprocessed_2_sumo?) Already exists?
So suggested action plan will then be:

  • Split off dataio and sumo bit, relocate to uploader?
  • Split off generation of workflow part to separate PR after the relocation is merged in

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2024

Codecov Report

Attention: Patch coverage is 94.84346% with 28 lines in your changes missing coverage. Please review.

Project coverage is 86.64%. Comparing base (25fa864) to head (2308db2).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/subscript/genertobs_unstable/_utilities.py 94.23% 14 Missing ⚠️
src/subscript/genertobs_unstable/_datatypes.py 92.38% 8 Missing ⚠️
src/subscript/genertobs_unstable/_writers.py 97.84% 3 Missing ⚠️
src/subscript/genertobs_unstable/parse_config.py 91.66% 2 Missing ⚠️
src/subscript/genertobs_unstable/main.py 96.87% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #704      +/-   ##
==========================================
+ Coverage   85.99%   86.64%   +0.64%     
==========================================
  Files          51       56       +5     
  Lines        6854     7397     +543     
==========================================
+ Hits         5894     6409     +515     
- Misses        960      988      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

return False


def is_percent_range(string):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem to check if the string contains %

Copy link
Collaborator

Choose a reason for hiding this comment

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

But I see that the checking is done before calling the function. Could probably update the docstring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants