-
Notifications
You must be signed in to change notification settings - Fork 68
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
IO changes #134
IO changes #134
Conversation
Simplifying and extending `from_csv`. Initial test uses `np.genfromtext` internally. Explicitly expects to be told which columns the `top`, `base` and/or `thickness` are in. Other columns will be added to a dictionary from which the intervals will be created. Addresses, but does not entirely close #128
Expanded docs. The will need to be restyled to match the rest of the library, but this will work for now. Added a series of FutureWarnings for args that we want to remove going forward. Added some new args.
Updated docs and args to be the same. Changed version for deprecation to 0.9.1.
Changed tests to use new `from_csv` method. The tests were passing in strings. These no need to be explicitly converted to `io.StringIO` objects, along with the `names=True`.
Removed extra args. These are not currently being used, but we might want them later. No need for them for now.
Removed old `from_csv` function - it is no longer being used anyway.
@kwinkunks How do you feel about Having either As it is, the code is using |
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! |
I recall it being ready, with no major changes to the user side, but will take another, proper look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to try integrating this today. I think we might have to deprecate more arguments, and probably write more tests.
Yeah, it definitely will need additional tests, or changes to existing ones. |
Bottom line: it's not working, and it's going to take quite some refactoring. Knee-deep in it now. |
I think I am going to have to back it out --- I can't get it to work. It needs looking at again. |
This reverts commit 58ca021.
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 usenp.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 thenames
arg should fix this.There are a fair few that will be deprecated, going forwards.
This will address #128, and part of #46