allow conversion (with loss of precision) for arbitrary precision POSIXct objects #533
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #533 +/- ##
==========================================
+ Coverage 80.15% 80.22% +0.07%
==========================================
Files 26 26
Lines 1693 1699 +6
==========================================
+ Hits 1357 1363 +6
Misses 336 336 ☔ View full report in Codecov by Sentry. |
src/convert/datetime.jl
Outdated
| function rcopy(::Type{DateTime}, x::Float64) | ||
| ms = ((isnan(x) ? 0 : x) + 62135683200) * 1_000 | ||
| if !isinteger(ms) | ||
| @warn "Precision lost in conversion to DateTime" |
There was a problem hiding this comment.
Though R users are accustomed to warnings, they're rather unusual for Julia. Perhaps it would make sense for this to be either an error or a @debug message? Any idea what the impact would be of the precision loss in the worst case?
There was a problem hiding this comment.
Hmmmm.... erroring is in some sense the current behavior, but we could add a more meaningful error message that the user needs to round to millisecond precision first.
R essentially allows for infinite precision here (I couldn't find a documented precision), but Julia restricts this to milliseconds. So you're never more than 1ms off, but whether that matters depends a lot on application domain. Looking at the POSIX roots of this, POSIX mostly cares about seconds, so 🤷.
I think @debug is fine. It's better rounding than what a user would get if they rounded in R, then did the conversion to Julia:
R> x <- as.POSIXct('2020-10-09 12:09:46.1234')
R> round(x, "millisecond")
Error in match.arg(units) :
'arg' should be one of “secs”, “mins”, “hours”, “days”, “months”, “years”
There was a problem hiding this comment.
Any idea how common it is to have POSIXct values in R with meaningful sub-millisecond precision? At any rate, it could be worth calling this out somewhere in the documentation.
src/convert/datetime.jl
Outdated
| if !isinteger(ms) | ||
| @debug "Precision lost in conversion to DateTime" | ||
| end | ||
| ms = round(Int, ms) |
There was a problem hiding this comment.
Since you're rounding anyway, you could skip the trunc that isinteger does and just check for equality of values pre- and post-round. Doesn't really matter though.
Co-authored-by: Alex Arslan <[email protected]>
|
|
||
| !!! note "Conversions to `DateTime` are given in UTC!" | ||
| `POSIXct` stores times internally as UTC with a timezone attribute. | ||
| The conversion to `DateTime` necessarily strips away timezone information, resulting in UTC values. |
There was a problem hiding this comment.
If POSIXct has a notion of timezone, I wonder if it should actually be represented on the Julia side as a ZonedDateTime rather than a DateTime. That'd be a breaking change but something possibly worth considering, at least if I understand the situation correctly.
closes #396