-
Notifications
You must be signed in to change notification settings - Fork 10
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
Dask-ml datasets.py related changes #36
Comments
cc @gpfreitas |
I introduced some TODOs into @gpfreitas PR. In order to get the tests passing, we needed to:
Here is the commit where the TODO locations were introduced: 5e3c756 Comments from @gpfreitas on the subject:
|
In the end, it seemed to me the best way forward was to use the API I wanted as a "low-level" API, that is more strict (no *args, **kwargs) and use that to implement the higher level api that Peter wanted (through a wrapping function called The main issue at the API level here is that there are keyword arguments in the various Example:
This is not exclusive to the dataframe conversion. Converting to MLDatasets or xarray.Dataset objs have the same issue (naming dimensions, etc.) This may be the best we can do to provide a simple one-liner for generating data, but I'm not sure. I don't have any ideas that are strictly better than what we have. |
After speaking with @gpfreitas, it might make sense for us to refactor our approach, so that Python 2 compatibility and integration with dask-ml become simpler. Here are some ideas:
|
See this issue 67 in dask-ml. When that issue is addressed, fix the dask-ml imports in xarray_filters and their usage in datasets.py of this repo. We had intended to cover that in #20 but needed to cover dask-ml 67 first.
The text was updated successfully, but these errors were encountered: