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

Adapt to be used directly with fmu-ensemble #207

Merged
merged 7 commits into from
Jan 25, 2021
Merged

Adapt to be used directly with fmu-ensemble #207

merged 7 commits into from
Jan 25, 2021

Conversation

berland
Copy link
Collaborator

@berland berland commented Nov 19, 2020 •
edited
Loading

Based on what is needed to pass tests in fmu-ensemble

Necessary for able equinor/fmu-ensemble#166

  • Tests for the changes in ecl2df must be transferred from fmu-ensemble's test suite.
  • Pylinting

Bonus changes:

  • Deprecate non-datetime-index as return type from df()
  • Refactor/deletion for code readability, normalize_dates() has been removed (the function is assumed to be private)

@berland berland mentioned this pull request Nov 19, 2020
Open
6 tasks
@berland berland marked this pull request as ready for review December 23, 2020 17:44
@berland berland changed the title WIP: Adapt to be used directly with fmu-ensemble Adapt to be used directly with fmu-ensemble Dec 28, 2020
@berland berland mentioned this pull request Dec 28, 2020
Closed
@berland berland force-pushed the fromfmuensemble branch 2 times, most recently from 76247d2 to 199f296 Compare January 22, 2021 11:50
@berland berland requested review from asnyv and removed request for asnyv January 22, 2021 11:52
@codecov-io
Copy link

codecov-io commented Jan 22, 2021 •
edited
Loading

Codecov Report

Merging #207 (32515aa) into master (0a6aff0) will increase coverage by 0.10%.
The diff coverage is 88.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
+ Coverage   91.29%   91.40%   +0.10%     
==========================================
  Files          22       22              
  Lines        2781     2804      +23     
==========================================
+ Hits         2539     2563      +24     
+ Misses        242      241       -1
Impacted Files Coverage Δ
ecl2df/summary.py 93.36% <88.75%> (+1.34%) ⬆️

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 0a6aff0...32515aa. Read the comment docs.

jondequinor
jondequinor reviewed Jan 22, 2021
View reviewed changes
ecl2df/eclfiles.py Outdated Show resolved Hide resolved
jondequinor
jondequinor reviewed Jan 22, 2021
View reviewed changes
ecl2df/summary.py Outdated Show resolved Hide resolved
jondequinor
jondequinor reviewed Jan 22, 2021
View reviewed changes
ecl2df/summary.py Outdated Show resolved Hide resolved
@berland
Adapt to be used as smry engine for fmu-ensemble
ff8aa8f
@berland
datetime=False in df() will trigger FutureWarning
c450c58 
This is done to later be able to change the API, to obtain
cleaner code and more API predictability.

Some refactoring for code readability (pylint)
@berland
Remove function normalize_dates()
f4ba456 
This function is only used once, and is better left by
directly calling the Pandas API by the caller.
@berland
Fixes after review
548b56c
jondequinor
jondequinor reviewed Jan 22, 2021
View reviewed changes
ecl2df/summary.py Outdated Show resolved Hide resolved
jondequinor
jondequinor reviewed Jan 22, 2021
View reviewed changes
ecl2df/summary.py Outdated Show resolved Hide resolved
jondequinor
jondequinor reviewed Jan 22, 2021
View reviewed changes
ecl2df/summary.py Outdated Show resolved Hide resolved
jondequinor
jondequinor reviewed Jan 22, 2021
View reviewed changes
ecl2df/summary.py Outdated
@@ -86,7 +111,7 @@ def resample_smry_dates(
Options for timeresampling are
'daily', 'monthly' and 'yearly'.
'last' will give out the last date (maximum),
Copy link

Choose a reason for hiding this comment

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

Document first as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

jondequinor
jondequinor approved these changes Jan 22, 2021
View reviewed changes
Copy link

@jondequinor jondequinor left a comment

Choose a reason for hiding this comment

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

Technically this looks good to me. Can't really comment on the functional. Maybe you want a second pair of eyes?

@berland
Copy link
Collaborator Author

berland commented Jan 25, 2021

Technically this looks good to me. Can't really comment on the functional. Maybe you want a second pair of eyes?

Thanks! I'll see if @asnyv has any further comments before merge.

@asnyv
Copy link
Collaborator

asnyv commented Jan 25, 2021

I think it looks ok @berland
Doesn't seem like we have tweaked on existing behavior (except future warning)? And existing tests ok.

@berland berland merged commit 2ed6f7e into master Jan 25, 2021
@berland berland mentioned this pull request Jan 26, 2021
Closed
@berland berland deleted the fromfmuensemble branch January 27, 2021 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jondequinor jondequinor jondequinor approved these changes

@BryanLaura BryanLaura BryanLaura approved these changes

@HansKallekleiv HansKallekleiv Awaiting requested review from HansKallekleiv

@asnyv asnyv Awaiting requested review from asnyv

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

Successfully merging this pull request may close these issues.
5 participants
@berland @codecov-io @asnyv @jondequinor @BryanLaura