-
Notifications
You must be signed in to change notification settings - Fork 38
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
Fixes retrieval of chunk sizes for datasets #83
Conversation
Codecov Report
@@ Coverage Diff @@
## master #83 +/- ##
==========================================
- Coverage 79.62% 78.65% -0.98%
==========================================
Files 3 3
Lines 324 328 +4
Branches 59 61 +2
==========================================
Hits 258 258
- Misses 43 46 +3
- Partials 23 24 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Amazing! Thanks for this fix @tomchor.
xmovie/core.py
Outdated
"xmovie presets do not yet support the input of xr.Datasets. \ | ||
In order to use datasets as inputs, set `input_check` to False. \ | ||
Note that this requires you to manually set colorlimits etc." | ||
"xmovie presets do not yet support the input of xr.Datasets.\nIn order to use datasets as inputs, set `input_check` to False.\nNote that this requires you to manually set colorlimits etc." |
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.
should we modify this to 'fully supports xr.Datasets'? Might be more true to the code? Just a suggestion
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 agree and I modified it :)
Feel free to merge whenever you're ready! |
Although (since I've heard about this bug from other people too) maybe it'd be good to tag a patch release immediately as well? |
Do you want to add an entry to |
Done! |
Phenomenal. Merging now. |
I guess an additional test (or a parametrization for the existing ones) would have been ideal. In case you are up for adding that, I would wait, but not a big deal in my view. |
I agree, but that would take considerably longer and I'm going to take some time off starting today. I thought it was better to get the bug fix out as soon as possible. I can help you out in the future if you want to add tests for datasets and make that aspect of |
Awesome, thanks for the offer. Enjoy your vacation! |
Closes #82
This fixes the issue for me. Although I did notice some minor issues when using Datasets (like the package giving me this warning even with the keyword
fieldname
:UserWarning: No fieldname supplied. Defaults to air
), but I think those are best left for future PRs.