Patrick/move out las reader writer #4

patrick-reinhard · 2021-09-27T16:07:17Z

Note: First merge #3 before merging this.

Tests are passing
Moving out LAS reader and writer functionality from Curve and Well class to the las.py module.
Tried leaving interfaces intact. Only interface change is that the Project.header attribute is now a pd.DataFrame instead of Header class. Adapted functions and methods to accommodate for Project.header being a pd.DataFrame
Redirect from_lasio() to from_las() and add deprecation warning to from_lasio().
Mapping a LAS file (version <= 2.0) 1 to 1 to a pd.DataFrame by splitting the header and the data section from the LAS in two dataframes:

  datasets = {'Curves': (data, header))

  data = pd.DataFrame({
      'DEPT': [100.0, 101.0, 102.0],
      'GR': [80.0, 85.0, 82.0],
      'DEN': [2.10, 2.15, 2.20]
  })

  header = pd.DataFrame({
      'original_mnemonic': ['VERS', 'WRAP', 'STRT', 'STOP', 'STEP', 'DEPT', 'GR', 'DEN', ''],
      'mnemonic': ['VERS', 'WRAP', 'STRT', 'STOP', 'STEP', 'DEPT', 'GR', 'DEN', ''],
      'unit': ['', '', 'M', 'M', 'M', 'M', 'GAPI', 'g/cm3', '']
      'value': [2.0, 'NO', 100.0, 102.0, 1.0, '', '', '', '']
      'descr': ['Version 2.0', 'One line per depth step', '', '', '', 'DEPTH', 'Gamma Ray', 'Density', 'Comment']
      'section': ['Version', 'Version', 'Well', 'Well', 'Well', 'Curves', 'Curves', 'Curves', 'Other']
  })

Already accommodated datasets design for LAS 3.0 (2D and 3D data) but lasio work on this is not yet finished. LAS <=2.0 you can map a las file 1 to 1 to a dataframe. For LAS 3.0 you can have multiple sections and runs that each map 1 tot 1 to a dataframe:

datasets = {
        'Curves':   (data, header), # for LAS 1.2 & LAS 2.0
        'ASCII':    (data, header), # for LAS 3.0
        'Drilling': (data, header), # for LAS 3.0
        'Core[1]':  (data, header), # for LAS 3.0 - Run 1
        'Core[2]':  (data, header)  # for LAS 3.0 - Run 2
    }

…-reader-writer

…ve classes

…-reader-writer

welly/las.py

welly/well.py

fkiraly-shell

Nice piece of work!

I've looked only at curve, well, las and the tests.

Major comments:

this PR seems to contain changes to plotting that are unrelated to the claimed content (las writer)? This seems problematic, especially since the other PR is still being worked on. What is the branch dependency and merge sequence here?
I'd put the las module not in the root but in a data_io folder or similar.
there are also changes to file level fixture paths. Why is that? Should this not "safer" in a separate PR, to avoid doing too much in one place? Especially since it is also touching tests and methods unrelated to the LAS IO concern? Or is there a good reason to do this here?
read/write tests can act up due to paths being interpreted differently on different operating systems. Are we sure that the writing works correctly for every developer setup?
there is still a lot of read/write/formatting logic in the well class, in from_las and to_las and add_curves_from_lasio. Do we really need that in there? It feels like it should be a loading/IO concern, and calling a method that creates curves from the output of the loaders in the las module.
similar in curve.from_lasio_curve, do we really want all that stuff in there?

patrick-reinhard · 2021-10-06T15:46:07Z

this PR seems to contain changes to plotting that are unrelated to the claimed content (las writer)? This seems problematic, especially since the other PR is still being worked on. What is the branch dependency and merge sequence here?

Merging sequence is #3, then this PR, then #6. #3 Is already merged into this one and there are no conflicts.

I'd put the las module not in the root but in a data_io folder or similar.

I'd like to stick to the current structure without subfolders.

there are also changes to file level fixture paths. Why is that? Should this not "safer" in a separate PR, to avoid doing too much in one place? Especially since it is also touching tests and methods unrelated to the LAS IO concern? Or is there a good reason to do this here?

I think it is safe as we are sequentially merging PRs one-by-one. Needed to adjust a few tests to accommodate for the new header object type (pd.DataFrame instead of Header)

read/write tests can act up due to paths being interpreted differently on different operating systems. Are we sure that the writing works correctly for every developer setup?

Added utils.to_filename(path) and lasio is also taking care of that here

there is still a lot of read/write/formatting logic in the well class, in from_las and to_las and add_curves_from_lasio. Do we really need that in there? It feels like it should be a loading/IO concern, and calling a method that creates curves from the output of the loaders in the las module.

Yes I agree. We could move that out. I can also address that in the next #6 PR. This PR is really only about moving out the reader/writer logic (from and to disk) while trying to keep interfaces intact.

similar in curve.from_lasio_curve, do we really want all that stuff in there?

This I will address in #6 where I change the curve object. Didn't want to touch the curve construction in this PR.

…sion

justyna-przybysz

Hi Patrick,

this looks really good. I had few comments related to doc. DO you have a notebook or .py file for testing to share?

welly/las.py

welly/well.py

…logic to las.py

patrick-reinhard · 2021-10-12T12:46:36Z

@fkiraly-shell
I've pushed changes to address your feedback. There are 2 things I would like to do in #6 because it will be influenced by that PR and therefore making the changes now will create extra rework:

Implementing well.to_datasets()
Moving out construction logic from well.from_datasets().

I've added TODO tags to keep track of those.

Requesting your review and approval.

wenting-x and others added 22 commits September 24, 2021 11:08

Add plot.py

1862f13

Use plotting functions from plot.py

3adea23

Use plotting functions for Well from plot.py

9c0bb0a

Use plotting functions for Curve from plot.py

b013576

Add unit tests to plotting functions in Project, Well & Curve

d064c6e

Remove unused parameter

fae0251

Add unit tests for qc functions

ce3c04f

Move out qc functions to quality.py

2366d06

Add unit tests for Curve qc functions

d3df06e

Move out qc functions from Curve

cd36182

Move qc orchestration functions to top, complete docstring

0330305

Group imports

8d10e7e

Group imports

cebe22a

update tests

38675b3

adapt methods for well header dataframe constructed by from_las()

d9a6c6e

add pycharm configuration files to gitignore

bdaf779

move out reader/writer

7782e87

move test assets (example files) to assets dir

4592dac

adapt well for header dataframe instead of header class

2f0a2ce

formatting pep8

2c992a2

add pandas to requirements

b26fd36

Merge branch 'wenting/move-out-plotting-qc' into patrick/move-out-las…

f9fb634

…-reader-writer

patrick-reinhard requested review from fkiraly-shell, wenting-x and justyna-przybysz September 27, 2021 16:13

patrick-reinhard mentioned this pull request Sep 28, 2021

Welly refactor plan #5

Open

8 tasks

wenting-x added 4 commits October 1, 2021 10:48

Improve docstring

feb6f7d

Call function from plot.py instead of the function from the Well class

5b78026

Call functions from quality.py instead of the functions from Well/Cur…

52cd660

…ve classes

Add example to tests and alias description

0f86d95

Merge branch 'wenting/move-out-plotting-qc' into patrick/move-out-las…

cd4380f

…-reader-writer

patrick-reinhard mentioned this pull request Oct 4, 2021

Patrick/replace curve object with dataframe #6

Draft