-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Open mfdataset enchancement #9955
base: main
Are you sure you want to change the base?
Conversation
Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient. |
I'm not the expert, but this looks reasonable! Any other thoughts? Assuming no one thinks it's a bad idea, we would need tests. |
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 it is a good idea.
But the way it is implemented here seems overly complicated and repetitive.
I would suggest to revert the logic: first build up the list wrapped in a single try
and then handle the three cases in the except
block.
Co-authored-by: Michael Niklas <mick.niklas@gmail.com>
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.
Almost there.
Also, we should add tests for this.
@headtr1ck Thanks for the suggestions. I have added two tests ( |
Hi @headtr1ck, I have been thinking about the handling of |
@max-sixty Can you please go through the PR. Thanks! |
I'm admittedly much less familiar with this section of the code. nothing seems wrong though! I think we should bias towards merging, so if no one has concerns then I'd vote to merge could we fix the errors in the docs? |
It seems like one test failed |
whats-new.rst
Added new argument in
open_mfdataset
to better handle the invalid files.