373 slices_restore will restore Dates and POSIXT classes #432
Conversation
|
Code Coverage SummaryDiff against mainResults for commit: 7f0020c Minimum allowed coverage is ♻️ This comment has been updated with latest results |
|
I am concerned about time zones. |
|
Indeed |
Merge branch '373_restore_classes@main' of https://github.com/insightsengineering/teal.slice into 373_restore_classes@main # Conflicts: # R/teal_slice-store.R
|
checkout last commit, where I tried to unify stored POSIX to UTC. When storing we change the timezone so the display is displayed in UTC, and when restoring we then know we need to set timezone to UTC. The print/format is different after the restorage but the time representation (as number of seconds from 1970-01-01) is the same |
|
TODO: review if |
There was a problem hiding this comment.
Great job on this feature!! 💯
I thought on adding some tests. See branch 373_add_tests that merges to this one.
Note: it skips the last 2 tests for now and might be enabled/removed depending on feedback
Co-authored-by: André Veríssimo <[email protected]> Signed-off-by: Marcin <[email protected]>
Co-authored-by: André Veríssimo <[email protected]>
|
Thanks for the branch with new tests. Just merged to this branch. Still need to figure out what happens for factors |
|
Hey @averissimo and @chlebowa the issue for Similar issue appears for |
There was a problem hiding this comment.
It looks good to me 💯
I won't approve right now to wait for @chlebowa feedback as he commented before
I was a bit concern about using all(grepl(..., but after some tests I can see that teal_slices doesn't work well with a mix bag of selected
time_stamps <- Sys.time() + c(-10 * 60 * 60 * 24, -30, 0)
time_stamps <- as.POSIXct(ceiling(as.double(time_stamps)), tz = "UTC")
# Converts Date to POSIXct
teal_slices(
teal_slice(
dataname = "ADSL",
varname = "EOSDTM",
selected = c(time_stamps, Sys.Date()),
fixed = TRUE
)
)
# Converts POSIXct to Date
teal_slices(
teal_slice(
dataname = "ADSL",
varname = "EOSDTM",
selected = c(Sys.Date(), time_stamps),
fixed = TRUE
)
)
# Assumes all as strings
tss <- teal_slices(
teal_slice(
dataname = "ADSL",
varname = "EOSDTM",
selected = c("A string", time_stamps, Sys.Date()),
fixed = TRUE
)
)
# Fails to convert string to Posix.ct (acceptable)
tss <- teal_slices(
teal_slice(
dataname = "ADSL",
varname = "EOSDTM",
selected = c(time_stamps, Sys.Date(), "A string"),
fixed = TRUE
)
)
|
@averissimo please bare in mind, that we will store and restore data provided by end app users and app developers and for those it is hard to mix such structure. End app users will have shiny inputs to provided the data, and the app developer will rather start with direct names of levels or even pull names directly from data. I think mixing classes is unlikely to happen |
|
@averissimo The coercions you listed are done by |
|
@chlebowa & @m7pr I'm always a bit pessimistic with weird edge cases 😅 The only semi-realistic scenario would be if the user deliberately wants to break the application. tss <- teal_slices(
teal_slice(
dataname = "ADSL",
varname = "EOSDTM",
selected = c(
"beta 2023-09-11",
"release candidate 2023-09-21",
"release 2023-09-21"
),
fixed = TRUE
)
)
slices_store(tss, slices_path)
tss_restored <- slices_restore(slices_path)
# Error in charToDate(x) :
# character string is not in a standard unambiguous format |
|
This could be protected if we look for a full regex match, however, I don't think we should add the complexity (and possible new bugs from it). # ...
if (!is.null(slice[[field]])) {
date_partial_regex <- "^[0-9]{4}-[0-9]{2}-[0-9]{2}"
time_stamp_regex <- paste0(date_partial_regex, "T[0-9]{2}:[0-9]{2}:[0-9]{2}$")
slice[[field]] <-
if (all(grepl(paste0(date_partial_regex, "$"), slice[[field]]))) {
if (all(grepl(time_stamp_regex, slice[[field]]))) {
as.POSIXct(gsub("T|Z", " ", slice[[field]]), tz = "UTC")
} else {
as.Date(slice[[field]])
}
} else {
slice[[field]]
}
}
# ... |
|
Hey @gogonzo and @chlebowa - thanks for the highly productive and fruitful discussion today. The final proposition of the usage looks like below: > x <- teal_slice(
+ dataname = "ADSL",
+ varname = "COUNTRY",
+ selected = as.POSIXct(c("2023-06-29 00:00:01", "2023-06-29 00:00:02"), tz = "EST"),
+ fixed = TRUE
+ )
> y <- teal_slices(x)
> y
{
"slices": [
{
"dataname" : "ADSL",
"varname" : "COUNTRY",
"id" : "ADSL COUNTRY",
"selected" : ["2023-06-29 00:00:01 EST", "2023-06-29 00:00:02 EST"],
"fixed" : true,
"anchored" : false,
"multiple" : true
}
],
"attributes": {
"allow_add" : true
}
}
> slices_store(y, file = 'store.json')
> z <- slices_restore('store.json')
> z
{
"slices": [
{
"dataname" : "ADSL",
"varname" : "COUNTRY",
"id" : "ADSL COUNTRY",
"selected" : ["2023-06-29 00:00:01 EST", "2023-06-29 00:00:02 EST"],
"fixed" : true,
"anchored" : false,
"multiple" : true
}
],
"attributes": {
"allow_add" : true
}
} TODOOne thing to be fixed (ON TODO LIST) is that some timezones are named differently in x <- teal_slice(
+ dataname = "ADSL",
+ varname = "COUNTRY",
+ selected = as.POSIXct(c("2023-06-29 00:00:01", "2023-06-29 00:00:02"), tz = "NZ"),
+ fixed = TRUE
+ )
> y <- teal_slices(x)
> y
{
"slices": [
{
"dataname" : "ADSL",
"varname" : "COUNTRY",
"id" : "ADSL COUNTRY",
"selected" : ["2023-06-29 00:00:01 NZST", "2023-06-29 00:00:02 NZST"],
"fixed" : true,
"anchored" : false,
"multiple" : true
}
],
"attributes": {
"allow_add" : true
}
}
> slices_store(y, file = 'store.json')
> z <- slices_restore('store.json')
Warning messages:
1: In strptime(xx, f, tz = tz) : unknown timezone 'NZST'
2: In as.POSIXct.POSIXlt(x) : unknown timezone 'NZST'
3: In strptime(x, f, tz = tz) : unknown timezone 'NZST'
4: In as.POSIXct.POSIXlt(as.POSIXlt(x, tz, ...), tz, ...) :
unknown timezone 'NZST'
> z
{
"slices": [
{
"dataname" : "ADSL",
"varname" : "COUNTRY",
"id" : "ADSL COUNTRY",
"selected" : ["2023-06-29 00:00:01 GMT", "2023-06-29 00:00:02 GMT"],
"fixed" : true,
"anchored" : false,
"multiple" : true
}
],
"attributes": {
"allow_add" : true
}
}
Warning message:
In as.POSIXlt.POSIXct(x, tz) : unknown timezone 'NZST'the same happens for CET that gets changed to CEST, where CEST is not recognized by Will work on that and will keep you posted. |
|
The validity of time zone codes is platform-dependent, I'm afraid. |
|
lemme investigate :) |
|
😠 this is an upstream bug from |
|
Ah, but we are stepping away from ISO8601 and trying to convert to strings by ourselves and store those. |
|
Oh my bad then. I missed it in the comments as the code doesn't seem to apply that If you're considering using the format(Sys.time(), format = "%Y-%m-%d %H:%M:%S %z", usetz = FALSE)
as.POSIXct(c("2023-06-29 17:02:01 +0200"), tryFormats = "%Y-%m-%d %H:%M:%S %z") |
|
This doesn't appear if we change the class of final restored object from x <- teal_slice(
+ dataname = "ADSL",
+ varname = "COUNTRY",
+ selected = as.POSIXct(c("2023-06-29 00:00:01", "2023-06-29 00:00:02"), tz = "CET"),
+ fixed = TRUE
+ )
> y <- teal_slices(x)
> y
{
"slices": [
{
"dataname" : "ADSL",
"varname" : "COUNTRY",
"id" : "ADSL COUNTRY",
"selected" : ["2023-06-29 00:00:01 CEST", "2023-06-29 00:00:02 CEST"],
"fixed" : true,
"anchored" : false,
"multiple" : true
}
],
"attributes": {
"allow_add" : true
}
}
> slices_store(y, file = 'store.json')
> z <- slices_restore('store.json')
> z
{
"slices": [
{
"dataname" : "ADSL",
"varname" : "COUNTRY",
"id" : "ADSL COUNTRY",
"selected" : ["2023-06-29 00:00:01 CEST", "2023-06-29 00:00:02 CEST"],
"fixed" : true,
"anchored" : false,
"multiple" : true
}
],
"attributes": {
"allow_add" : true
}
}
> class(shiny::isolate(z[[1]]$selected))
[1] "POSIXlt" "POSIXt" |
|
|
🤔 I would rigorously test if |
|
Will test it out today and let you know.
We can always do as.posixct(as.posixlt()) :) in restore
|
> as.POSIXlt("2023-10-08 08:46:40 UTC", format = "%Y-%m-%d %H:%M:%S")
[1] "2023-10-08 08:46:40 CEST"
> as.POSIXlt("2023-10-08 08:46:40 CEST", format = "%Y-%m-%d %H:%M:%S")
[1] "2023-10-08 08:46:40 CEST"
To have explicit amount of hour displayed after the timestamp. So now we only deal with numbers (and not characters that tend to change. > format(as.POSIXct(c("2023-06-29 00:00:01", "2023-06-29 00:00:02"), tz = "CET"), usetz = TRUE)
[1] "2023-06-29 00:00:01 CEST" "2023-06-29 00:00:02 CEST"
> as.POSIXct(format(as.POSIXct(c("2023-06-29 00:00:01", "2023-06-29 00:00:02"), tz = "CET"), usetz = TRUE), tz = 'CEST')
[1] "2023-06-29 00:00:01 GMT" "2023-06-29 00:00:02 GMT"
Warning messages:
1: In strptime(xx, f, tz = tz) : unknown timezone 'CEST'
2: In as.POSIXct.POSIXlt(x) : unknown timezone 'CEST'
3: In strptime(x, f, tz = tz) : unknown timezone 'CEST'
4: In as.POSIXct.POSIXlt(as.POSIXlt(x, tz, ...), tz, ...) :
unknown timezone 'CEST'
5: In as.POSIXlt.POSIXct(x, tz) : unknown timezone 'CEST' |
|
See, I don't get those warnings 🤔 |
|
I just pushed @averissimo proposition, but the restore restores the timestamp into the current timezone of the session, which is not ideal. I will need to revert it and work on short-codes of timezones x <- teal_slice(
+ dataname = "ADSL",
+ varname = "COUNTRY",
+ selected = as.POSIXct(c("2023-06-29 00:00:01", "2023-06-29 00:00:02"), tz = "UTC"),
+ fixed = TRUE
+ )
> y <- teal_slices(x)
> y
{
"slices": [
{
"dataname" : "ADSL",
"varname" : "COUNTRY",
"id" : "ADSL COUNTRY",
"selected" : ["2023-06-29 00:00:01 +0000", "2023-06-29 00:00:02 +0000"],
"fixed" : true,
"anchored" : false,
"multiple" : true
}
],
"attributes": {
"allow_add" : true
}
}
> slices_store(y, file = 'store.json')
> z <- slices_restore('store.json')
> z
{
"slices": [
{
"dataname" : "ADSL",
"varname" : "COUNTRY",
"id" : "ADSL COUNTRY",
"selected" : ["2023-06-29 02:00:01 +0200", "2023-06-29 02:00:02 +0200"],
"fixed" : true,
"anchored" : false,
"multiple" : true
}
],
"attributes": {
"allow_add" : true
}
}
> class(shiny::isolate(z[[1]]$selected))
[1] "POSIXct" "POSIXt" |
|
I wonder if |
|
👍 lubridate.. There's a reason why the meme about never working with timezones, it's full of problems. Is it a big problem to restore the timezone into the local one? |
|
It's not an issue at all for filtering. It's only a cosmetic visual
difference in print.
…On Wed, Oct 18, 2023, 15:29 André Veríssimo ***@***.***> wrote:
👍 lubridate.. There's a reason why the meme about never working with
timezones, it's full of problems.
Is it a big problem to restore the timezone into the local one?
—
Reply to this email directly, view it on GitHub
<#432 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A74AIEM62MSFBTAMJWMTKY3X77KVFAVCNFSM6AAAAAA3JX7FDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRYGQ3DKMRUG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…this assumption for store and restore
|
Ok guys, let's end this painful process. I know you are all sick and tired of timestamps already! Based on all edge-cases that we've noticed during this discussion, I assume there is no gold solution in here. I just made final changes and the one that I propose, at least in my opinion, is the least evil. Even though a timestamp can be passed in any timezone, we do convert it to Having UTC in print/format (hence store which uses format) allows us to restore > x <- teal_slice(
+ dataname = "ADSL",
+ varname = "COUNTRY",
+ selected = as.POSIXct(c("2023-06-29 00:00:01", "2023-06-29 00:00:02"), tz = "CET"),
+ fixed = TRUE
+ )
> y <- teal_slices(x)
> y
{
"slices": [
{
"dataname" : "ADSL",
"varname" : "COUNTRY",
"id" : "ADSL COUNTRY",
"selected" : ["2023-06-28 22:00:01 UTC", "2023-06-28 22:00:02 UTC"],
"fixed" : true,
"anchored" : false,
"multiple" : true
}
],
"attributes": {
"allow_add" : true
}
}
> slices_store(y, file = 'store.json')
> z <- slices_restore('store.json')
> z
{
"slices": [
{
"dataname" : "ADSL",
"varname" : "COUNTRY",
"id" : "ADSL COUNTRY",
"selected" : ["2023-06-28 22:00:01 UTC", "2023-06-28 22:00:02 UTC"],
"fixed" : true,
"anchored" : false,
"multiple" : true
}
],
"attributes": {
"allow_add" : true
}
}
> class(shiny::isolate(z[[1]]$selected))
[1] "POSIXct" "POSIXt" |
|
Seems so be working just fine and I see that checks have passed, but some test fail for me locally 🤔 |
|
I guess the tests should use
|
That explains it 😒 |
|
appended tests. thanks for local check. I think we should extend GITHUB R CMD CHECK with multiple R versions (and preferably multiple platforms) |
|
Raised a request to start running GHA Checks with multiple R versions https://github.com/insightsengineering/idr-tasks/issues/682 |
Closes #373
A very small proposition on how we can guess that a character contains a Date a POSIXT time. Up to be discussed as we might want to just use jsonlite::serializeJSON and jsonlite::unserializeJSON
The code
The code with results