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.

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

Continue work on transition of load_array and load_meta #31

Closed
PeterDSteinberg opened this issue Sep 22, 2017 · 6 comments
Closed

Continue work on transition of load_array and load_meta #31

PeterDSteinberg opened this issue Sep 22, 2017 · 6 comments
Assignees

Comments

@PeterDSteinberg
Copy link
Contributor

How do load_array and load_meta from earthio relate to dataset loading capabilities in xarray?

For examples:

  • Is there anything in the GeoTiff related logic for earthio that should be in xarray? The earthio GeoTiff reader (with rasterio) was a Phase I thing before xarray added GeoTiff support.
  • Do earthio's load_array and load_meta need to exist? Can everything they do now be effectively pushed into xarray? This may already be the case for the NetCDF reader.
@PeterDSteinberg
Copy link
Contributor Author

#17 is the same issue but more specific to GeoTiff file support.

@PeterDSteinberg
Copy link
Contributor Author

SHould it be called 'load_mldataset'?

@gbrener
Copy link
Contributor

gbrener commented Nov 3, 2017

Hey @PeterDSteinberg, responding to your last comment - do you think it would make sense to call this load_layers()? Since we support passing layer_specs (via a kwarg) and we've already centralized around the "layer" as a core abstraction? Just an idea. It might help us keep earthio decoupled conceptually from elm and xarray_filters. Happy to keep load_array() for now though, if you're not in favor.

In terms of integrating with xarray_filters / MLDataset, I was thinking we could add a MLDataset.load() method (or something similar), which could then call earthio.load_array() under the hood if necessary. That way we wouldn't have to return a data structure from earthio.load_array() that strictly depends on xarray_filters; perhaps it could be just a xr.Dataset or xr.DataArray? What do you think?

Regarding what we may be able to feed back to xarray from the GeoTiff work in earthio - if you recall the table I created a few months ago, I think we've closed the gap (to some extent) in xarray by preserving more of the GeoTiff metadata than was previously being collected. Also, the geotransform features - and I could be wrong here - were added to MLDataset with the Step work we did a few weeks ago. It might be a good idea to transition to using the xarray rasterio backend at some point if we want to reduce our maintenance burden.

@PeterDSteinberg
Copy link
Contributor Author

  • I like load_layers (plural form makes more sense + agreed that layers is more consistent with other work).
  • Let's go with your suggestion of having load_layers return xr.Dataset (they all return MLDataset now, I'm pretty sure, so it will be simple to change to the more general xr.Dataset
  • MLDataset.load makes sense to me (importing earthio as an optional import wherever it is used in xarray_filters
  • Thanks for reminder on the GeoTiff capabilities table - Let's make a notebook documenting that (I'll create a separate docs issue reminder - I think overall one of the potential confusions of new users is what returns/uses DataArray vs Dataset vs MLDataset)

@gbrener
Copy link
Contributor

gbrener commented Nov 3, 2017

Cool, thanks for the feedback. I'll get started on the implementation shortly.

@gbrener
Copy link
Contributor

gbrener commented Nov 6, 2017

Closing this issue now that #39 is merged. Opened ContinuumIO/xarray_filters#43 to identify the remaining work left to be done, which is in the xarray_filters library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants