-
Notifications
You must be signed in to change notification settings - Fork 13
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
Expose data_vars and coords arguments to xr.open_mfdataset in DataLoaders #240
Expose data_vars and coords arguments to xr.open_mfdataset in DataLoaders #240
Conversation
Thanks @spencerkclark! Looks good to me. Go ahead and merge at your discretion. |
@spencerahill great! I'm considering making |
@spencerkclark yes definitely. Thanks for the in-depth troubleshooting on that one. Ping me once that's in and ready and I'll give it another look over |
@spencerahill @micahkim23 this should be ready for another review. |
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.
A couple minor points + one request. Otherwise looks good!
aospy/data_loader.py
Outdated
else: | ||
int_names = (set(GRID_ATTRS.keys()) - | ||
set(TIME_VAR_STRS)) | ||
int_names = GRID_ATTRS.keys() | ||
grid_attrs_in_ds = set(int_names).intersection(set(ds.coords) | | ||
set(ds.data_vars)) | ||
ds.set_coords(grid_attrs_in_ds, inplace=True) |
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.
Would you mind taking care of #244 while you're at it by removing this use of inplace
?
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.
Sure thing, that should be straightforward.
aospy/data_loader.py
Outdated
else: | ||
int_names = (set(GRID_ATTRS.keys()) - | ||
set(TIME_VAR_STRS)) | ||
int_names = GRID_ATTRS.keys() |
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.
Could just use GRID_ATTRS.keys()
directly rather than defining int_names
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.
I think this was the way it was prior to #199 (I think because of line length in the following line). Happy to change it though.
aospy/data_loader.py
Outdated
self._set_default('preprocess_func', preprocess_func, | ||
lambda ds, **kwargs: ds) | ||
|
||
def _maybe_inherit(self, template, attr, value): |
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.
Since this and _set_default
aren't using any of the object's methods or attributes, I think it would be cleaner to define these at the module level as functions: def _maybe_inherit(obj, template, attr, value):
etc.
Also, are the two different enough to warrant separate definitions? Am I correct that _set_default(obj, attr, value, getattr(template, attr)) == _maybe_inherit(obj, template, attr, value)
?
If you do keep both, I'd suggest making the signatures more homogenous: obj, attr, value, template/default.
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.
Indeed! Good catch. I'll clean these up when I get a chance.
Appveyor seems to have been well-behaved today :). @spencerahill let me know if my changes look good to you. |
Quick, let's merge before it gets tempermental! 😜 Thanks @spencerkclark , this is great. |
This option ultimately did make it in to xarray version 0.10.0, so we can now clean up the logic required to work around the issue discussed here #199 (comment).