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

Use ecl2df for summary file extraction #182

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Use ecl2df for summary file extraction #182

wants to merge 3 commits into from

Conversation

berland
Copy link
Collaborator

@berland berland commented Nov 19, 2020

API CHANGE

  • realization.get_smry() returns a dummy index always, with DATE in its own column. This is more in line with the ensemble module.
  • cache_eclsum is removed entirely from the API.

Left to do :

  • Remove the eclsum concept entirely from realization.py, it should be handled through the EclFiles object
  • Rethink the caching?. Ensure that it is possible to load thousands of realizations by turning off caching.
  • Make changes to ecl2df.summary for what is needed, maybe some of the column key support functions.
  • Does the UNSMRY file discovery make sense when EclFiles is doing the work? Maybe.
  • Merge Adapt to be used directly with fmu-ensemble res2df#207 (and publish to pypi) for fmu-ensemble tests to pass in CI
  • Get metadata from DataFrame.attrs (via ecl2df).

The 3D grid access functions is also a candidate for refactoring, if the functionality is still needed.

@ertomatic
Copy link

Can one of the admins verify this patch?

@codecov-io
Copy link

codecov-io commented Mar 9, 2021

Codecov Report

Merging #182 (9703e48) into master (868bc9d) will increase coverage by 0.52%.
The diff coverage is 97.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
+ Coverage   83.08%   83.61%   +0.52%     
==========================================
  Files          14       14              
  Lines        2986     2948      -38     
==========================================
- Hits         2481     2465      -16     
+ Misses        505      483      -22     
Impacted Files Coverage Δ
src/fmu/ensemble/observations.py 83.48% <ø> (ø)
src/fmu/ensemble/realizationcombination.py 76.42% <80.00%> (+0.16%) ⬆️
src/fmu/ensemble/realization.py 89.02% <94.33%> (+2.42%) ⬆️
src/fmu/ensemble/ensemble.py 85.87% <100.00%> (-0.62%) ⬇️
src/fmu/ensemble/ensemblecombination.py 92.00% <100.00%> (+0.57%) ⬆️
src/fmu/ensemble/ensembleset.py 79.36% <100.00%> (+0.68%) ⬆️
src/fmu/ensemble/util/rates.py 100.00% <100.00%> (ø)
src/fmu/ensemble/virtualensemble.py 84.77% <100.00%> (+0.10%) ⬆️
src/fmu/ensemble/virtualrealization.py 89.55% <100.00%> (+0.61%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 868bc9d...9703e48. Read the comment docs.

* realization.get_smry() has changed to always return a dummy index
* cache_eclsum is pruned from fmu-ensemble.
@berland berland marked this pull request as ready for review March 9, 2021 12:42
This was referenced Mar 9, 2021
@berland berland linked an issue Mar 9, 2021 that may be closed by this pull request
@berland berland linked an issue Mar 9, 2021 that may be closed by this pull request
@berland berland requested a review from asnyv March 9, 2021 13:02
The argument column_keys is no longer accepted by get_smry_meta().
All smry metadata available to a realization/ensemble is now always
returned by this function.

This function is no longer the recommended practice for a client script
to obtain the relevant summary vector metadata, API users should use
the attrs["meta"] property of the returned summary dataframes instead.
@berland berland linked an issue Mar 11, 2021 that may be closed by this pull request
ensemble.get_smr() will no longer ensure that the dates returned
are identical in all realization, realizations with shorter timespans
will not be extrapolated as they were before.

This also affects get_smry_stats(), for which realizations
which failed prematurely earlier had zero rates, affecting statistical
profiles. Now these realization with truncated time-series will only
affect the statistical profiles in the time-range they have data for.
Comment on lines +868 to +870
logger.warning(
"Multiple UNSMRY files found, consider turning off auto-discovery"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not directly related to this PR, but I think this warning in the future at least should tell which file that was chosen, but I am a bit tempted to not accept it at all, and rather fail. The behavior is very risky...

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