search

agilescientific / striplog

Lithology and stratigraphic logs for wells or outcrop.
https://code.agilescientific.com/striplog
Apache License 2.0
204 stars 69 forks source link

IO changes #134

Closed mtb-za closed 2 years ago

mtb-za commented 3 years ago

The first of a few projected PRs on this branch. Others should look at things like from_dict, from_json and so on.

This one makes changes to from_csv to use np.genfromtxt in the background. This allows for more flexibility, and a more robust set of customisation when reading in data.

We might still want to add some new args, for things like top, base columns. In theory, the use of the names arg should fix this.

There are a fair few that will be deprecated, going forwards.

This will address #128, and part of #46

coveralls commented 3 years ago

Coverage Status

Coverage decreased (-0.5%) to 74.932% when pulling 140a86c6e065da62c5c2c44a469046a91ae5954b on IO-changes into 0c68f63d645c5bb7a5cc73b9bdaa197c4fb3cc33 on master.

mtb-za commented 3 years ago

@kwinkunks How do you feel about names being a mandatory argument (even if the default is just True)? If we do not have that, then we need to explicitly ask for which column is which (for important columns like top), and then we can only make a generic name for the ones that we do not actually ask for, which is not very helpful.

Having either names=True, or names=['top', 'base', 'description'] would sidestep the above potential headache. names=True will take the first row (after any skip_header) as the column names, while the other passes them in explicitly. Making it mandatory should make the code cleaner and easier to understand.

As it is, the code is using top and base if names == None, but these are not being passed, because I took them out in a commit earlier. I can put them back if we decide not to go this route easily enough.

kwinkunks commented 2 years ago

Damn, I can't remember where we left this after the hackathon. Looking at it again, but ping me @mtb-za if you have anything to add!

mtb-za commented 2 years ago

I recall it being ready, with no major changes to the user side, but will take another, proper look.

mtb-za commented 2 years ago

Yeah, it definitely will need additional tests, or changes to existing ones.

kwinkunks commented 2 years ago

Bottom line: it's not working, and it's going to take quite some refactoring. Knee-deep in it now.

kwinkunks commented 2 years ago

I think I am going to have to back it out --- I can't get it to work. It needs looking at again.