-
Notifications
You must be signed in to change notification settings - Fork 51
fix Contains
for DateTime
and Center
locus.
#1037
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1037 +/- ##
==========================================
+ Coverage 84.03% 84.41% +0.37%
==========================================
Files 52 52
Lines 5136 5139 +3
==========================================
+ Hits 4316 4338 +22
+ Misses 820 801 -19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
What bug does this fix exactly? I'm pretty sure I've also run into rounding errors with datetime dimensions but I'm not sure if I ever made an issue EDIT: figured it out - you get this when trying to shiftlocus |
Still getting an error on this branch when selecting with a
|
The bug is that some selectors dont work on Regular Center Intervals lookups with DateTime becuase its not clear how to find the Pracitcally the bug is that trying e.g. Contains or Between currently just gives an error. (and no the PR isn't solved because I'm not really sure how to yet... one option could be just failing for Month/Year and getting the rest right as they are exact quantities? I dont know really |
For DateTime an option is that the One problem is that in Julia things like dividing by 2, mean, and middle are not defined on dates/datetimes. I don't think we can get around breaking this convention (at least internally). There is some discussion here JuliaLang/julia#54542 |
Yeah. But step sizes of days, hours and seconds are pretty well defined so we could at least make them work everywhere without ambiguity. |
Not sure how we never hit this before, but
Center
Contains
is broken for DateTime. Needs a testOh, its because its annoyingly hard to define what the step size
Month
orYear
(with leap years) means exactly when it doesn't start at the start of the period.We could try using each the weigted mean of the overlapped months, with the weight coming from the distance the center point is from either one? Just have to make sure it works out with no gaps.
The other reason this has never been a real problem is the only time you get Center time is in netcdf, and there is always an Explicit bounds matrix in that case, and that already works fine.
But @tiemvanderdeure maybe you have some thoughts on this