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 |
spencerkclark
changed the title
|
@spencerahill @micahkim23 this should be ready for another review. |
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.
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.
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.
Could just use GRID_ATTRS.keys() directly rather than defining int_names
There was a problem hiding this comment.
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.
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.
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).