Allow concat_dim_kwargs in THREDDSMergedSource
#34
Conversation
intake_thredds/source.py
Outdated
| @@ -89,14 +89,21 @@ def _open_dataset(self): | |||
| else: | |||
| break | |||
| path = self.path[i:] | |||
| if 'concat_dim' in self.xarray_kwargs: | |||
There was a problem hiding this comment.
should we maybe allow more args from http://xarray.pydata.org/en/stable/generated/xarray.open_mfdataset.html?
There was a problem hiding this comment.
I like the idea of doing dataset concatenation. I think it would be beneficial not to expose the concat_dim option as a standalone argument. 👍🏽 for expanding the api to support other concat options. How about exposing concatenation options as concat_kwargs in __init__? I am imagining having concat_kwargs=None to be the default (which means the user doesn't want us to do any concatenation). With concat_kwargs={....}, we then go ahead and run xr.concat(data, **concat_kwargs).
What do you think?
There was a problem hiding this comment.
I see two ways here:
1.) try to implement xr.open_mfdataset API: http://xarray.pydata.org/en/stable/generated/xarray.open_mfdataset.html
2.) implement xr.concat(list, args) API: http://xarray.pydata.org/en/stable/generated/xarray.concat.html
but as I compare all concat_kwargs appear in open_mfdataset.
I like your implementation suggestion.
There was a problem hiding this comment.
so concat_dim=... and xarray_kwargs for all other kwargs of open_mfdataset
CHANGELOG.md
Outdated
| @@ -5,6 +5,7 @@ | |||
| ### Features | |||
|
|
|||
| - Allow `xarray_kwargs` for :py:class:`~intake_thredds.THREDDSMergedSource` [#32](https://github.com/NCAR/intake-thredds/pull/32) ([@aaronspring](https://github.com/aaronspring)) | |||
| - Allow `concat_dim` for :py:class:`~intake_thredds.THREDDSMergedSource` [#34](https://github.com/NCAR/intake-thredds/pull/34) ([@aaronspring](https://github.com/aaronspring)) | |||
There was a problem hiding this comment.
how do I get the xr docs linked here?
|
Ok to merge @andersy005 ? |
There was a problem hiding this comment.
This looks great, @aaronspring 👍
I'm merging this shortly (sorry I took too long to get back to you)
| @@ -5,6 +5,7 @@ | |||
| ### Features | |||
|
|
|||
| - Allow `xarray_kwargs` for :py:class:`~intake_thredds.THREDDSMergedSource` [#32](https://github.com/NCAR/intake-thredds/pull/32) ([@aaronspring](https://github.com/aaronspring)) | |||
| - Allow `concat_kwargs` for :py:class:`~intake_thredds.THREDDSMergedSource` [#34](https://github.com/NCAR/intake-thredds/pull/34) ([@aaronspring](https://github.com/aaronspring)) | |||
There was a problem hiding this comment.
I usually generate the changelog via https://github-activity.readthedocs.io/en/latest/. This is fine too. I just wanted to let you know that updating the changelog isn't required :) ...
|
thanks for merging! |
|
Should we cut a new release? |
|
I think so. |
Closes #33