-
Notifications
You must be signed in to change notification settings - Fork 23
Fix datetime conversion bug #560
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 Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #560 +/- ##
==========================================
+ Coverage 69.80% 69.87% +0.07%
==========================================
Files 14 14
Lines 2113 2118 +5
==========================================
+ Hits 1475 1480 +5
Misses 638 638
🚀 New features to boost your workflow:
|
| tsteps = try | ||
| timedecode(ar[:], aratts["units"], lowercase(get(aratts, "calendar", "standard"))) | ||
| dec = timedecode(ar[:], aratts["units"], lowercase(get(aratts, "calendar", "standard")), prefer_datetime=false) | ||
| round_datetime.(dec) |
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.
This is breaking because it changes the output type to a DateTime. This function should only be called when the prefer_datetime keyword is true.
This is also the cause of the doc build failure.
| round_datetime.(dec) | |
| prefer_datetime && round_datetime.(dec) |
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.
unfortunately,
failed to run `@example` block in docs/src/UserGuide/select.md:45-49
│ ```@example subset
│ using CFTime
│ time1 = DateTime360Day(2001,01,16)
│ tos[time = At(time1)]
│ ```
│ exception =
│ promotion of types CFTime.DateTime360Day{CFTime.Period{Int64, Val{1}(), Val{-3}()}, Val{(1900, 1, 1)}()} and DateTime failed to change any arguments
│ Stacktrace:
│ [1] error(::String, ::String, ::String)
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.
Is this with my suggestions?
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.
Ahh, sorry is still a suggestion! Not merged yet. Sorry, my bad 😞. We need to apply the suggestion to see the effect then.
This fixes a bug similar to the one described by @felixcremer in rafaqz/Rasters.jl#1000 by rounding to the closest millisecond before converting time stamps to DateTime.