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

Change load_array() -> load_layers(), return xr.Dataset #39

Merged
merged 6 commits into from
Nov 6, 2017

Conversation

gbrener
Copy link
Contributor

@gbrener gbrener commented Nov 6, 2017

Addresses #31 by changing load_array() to load_layers(), and having the function return an xr.Dataset object instead of MLDataset.

Name changes, plus having load_layers() return an xr.Dataset object instead of an MLDataset object.
@PeterDSteinberg
Copy link
Contributor

One variable naming thing related to this PR:

  • Change the usage of es (formerly an ElmStore from Phase I approach) to dset. There may also be places where X is used and dset would be more consistent with xarray/xarray_filters naming.

@gbrener
Copy link
Contributor Author

gbrener commented Nov 6, 2017

@PeterDSteinberg, just made the es -> dset change. Wasn't able to find X being used anywhere, but I could've missed something.

@PeterDSteinberg
Copy link
Contributor

Searching shows just one docstring using X:

:X: xr.Dataset
here where :X: should be :dset:

@PeterDSteinberg
Copy link
Contributor

Otherwise - can merge when tests pass.

@gbrener
Copy link
Contributor Author

gbrener commented Nov 6, 2017

Ah, thanks for pointing that one out. Just pushed up the fixes; waiting on tests to pass.

@PeterDSteinberg
Copy link
Contributor

With changes in the environment.yml / conda.recipe, can this line now be removed from build_earthio_env.sh?

@gbrener
Copy link
Contributor Author

gbrener commented Nov 6, 2017

Yes, I believe so - just pushed up the change.

@gbrener gbrener merged commit e0ddc31 into master Nov 6, 2017
@gbrener gbrener deleted the load_layers branch November 6, 2017 22:04
@gbrener
Copy link
Contributor Author

gbrener commented Nov 6, 2017

Merged despite package upload failure - that issue will be addressed in a separate PR.

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

Successfully merging this pull request may close these issues.

2 participants