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

added mfdataset to opendap #113

Merged
merged 3 commits into from
Feb 1, 2022
Merged

Conversation

kthyng
Copy link
Contributor

@kthyng kthyng commented Jan 28, 2022

Hi! I would like to use mfdataset with opendap and I put it in really simply. I wasn't sure how to incorporate store like in open_dataset, if that is possible. Also, I ran tests locally but on the master branch I had 5 errors, and the same 5 errors with this change so I figured it was something to do with my local configuration rather than the changes I made.

@aaronspring
Copy link
Collaborator

I am surprised that * works on urls. Could you add a test for urlpath as list and another containing *?

Added tests for opendap with 1 link and with 2 links.
@kthyng
Copy link
Contributor Author

kthyng commented Jan 31, 2022

@aaronspring Ah, you are right about that — "*" will not work with opendap (I copied it uncritically from the open_netcdf function). I removed that.

I added a test for open_opendap with a single url and a list of two urls.

What do you think?

@aaronspring
Copy link
Collaborator

Ok. So the passing of kwargs for either mfdataset or dataset is now implicitly set by the type of urlpath. list makes mfdataset and allows parallel=True.

works for me.

Does that solution also work for your intake catalog with 127 datasets?

@martindurant
Copy link
Member

I'm only watching this thread, please ping me if you need anything

@kthyng
Copy link
Contributor Author

kthyng commented Jan 31, 2022

@aaronspring Yep! It works with a big list of remote files like as described in the intake-thredds issue except doesn't rely on intake-thredds.

Any idea why some checks were not successful on this PR?

@aaronspring
Copy link
Collaborator

upstream issues are not your fault. same problem in #109 mentioned also in rasterio/rasterio#2381

@kthyng
Copy link
Contributor Author

kthyng commented Feb 1, 2022

Ok, just let me know if I should do anything to move this forward! Thanks @aaronspring and @martindurant.

@aaronspring
Copy link
Collaborator

aaronspring commented Feb 1, 2022

I agree with this PR. But cannot merge as I am not a maintainer here. Can you @martindurant? Or should we wait until the rasterio issue is solved?

@martindurant
Copy link
Member

I suppose rasterio is unrelated, so I can merge this now. Would you like to be a maintainer here, @aaronspring ?

@martindurant martindurant merged commit 77bd753 into intake:master Feb 1, 2022
@aaronspring
Copy link
Collaborator

I'd be happy to join @martindurant

@kthyng
Copy link
Contributor Author

kthyng commented Feb 1, 2022

Thank you @aaronspring and @martindurant!

@aaronspring
Copy link
Collaborator

Thank you @kthyng for this addition

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.

3 participants