schochastics / timeless

A general purpose date(time) parser for R
https://schochastics.github.io/timeless/
Other
19 stars 2 forks source link

undesired automatic tz conversion #14

Closed chrisumphlett closed 5 months ago

chrisumphlett commented 5 months ago

I have some places where I use anytime that take a long time to convert on large data frames and I hope to be able to switch to this package. Great job. I was just testing on one of them and that mutate step went from 18 secs to 1 sec.

I did notice however that I didn't get the same results. anytime seems to do the conversion ignoring (or perhaps preserving) the time zone. timeless converts it to my local TZ from (what it assumes to be?) GMT. I'm not surprised based on the documentation on chronos() it says the default argument of tz = "" will do this. I'd like to argue for a different default -- somehow ignoring or preserving the timezone.

(tangent: I saw you have another issue open #10 about DST. FWIW I already thought this was working because while I didn't want my converted time to change, I happened to see that it did reflect DST.)

I don't think there is a timezone-- a chr can't have a timezone. So it seems like chronos() presumes GMT. It could presume the local tz by default and then convert to the local tz by default (thereby not changing the time by default). And then you could have from_tz and to_tz parameters in chronos(). That might give the package some ability to do in one step what some people may be doing in 2 steps now with anytime + something else to deal with tz.

Which actually I just remembered is me as well. I use this combination sometimes: lubridate::with_tz(anytime::anytime(webinar_planned_start_ts), "America/New_York")).

I don't know anything about rust at all, I'm assuming I would not have the ability to jump in and make a simple change easily nor to quickly understand what's happening. But at least trying to explain the issue here and give a general suggestion about how to deal with it.

schochastics commented 5 months ago

Thanks for raising this. This was somewhat on my radar but so far did not know how to approach it. I completely agree with you and I think I will try to implement it the way you suggested

schochastics commented 5 months ago

@chrisumphlett the dev version here should have the desired feature now.

the parameter tz now only sets a tz and the parameter to_tz converts from tz to to_tz. here is an example:

library(timeless)
bench_date[5]
#> [1] "2017-11-25T22:34:50Z"

# no tz means local tz
chronos(bench_date[5])
#> [1] "2017-11-25 23:34:50 CET"

# set a tz
chronos(bench_date[5], tz = "America/Los_Angeles")
#> [1] "2017-11-25 23:34:50 PST"

# convert from set tz
chronos(bench_date[5], tz = "America/Los_Angeles", to_tz = "CET")
#> [1] "2017-11-26 08:34:50 CET"

Created on 2024-03-28 with reprex v2.1.0

Could you give it a spin and report if it works as desired?

schochastics commented 5 months ago

given that the tz test fails on GA, I assume there is still work to be done

Edit: man, the error is even right there in my reprex :D

schochastics commented 5 months ago

ok it might be fixed now. Could you give it a try @chrisumphlett

library(timeless)
bench_date[5]
#> [1] "2017-11-25T22:34:50Z"

# no tz means set local tz
chronos(bench_date[5])
#> [1] "2017-11-25 22:34:50 CET"

# set a tz
chronos(bench_date[5], tz = "America/Los_Angeles")
#> [1] "2017-11-25 22:34:50 PST"

# convert from set tz
chronos(bench_date[5], tz = "America/Los_Angeles", to_tz = "CET")
#> [1] "2017-11-26 07:34:50 CET"

Created on 2024-03-29 with reprex v2.1.0

chrisumphlett commented 5 months ago

I'm getting an error using devtools::install_github() that I've not seen before. Does this mean anything to you?

# `rustc` adds `-lgcc_eh` flags to the compiler, but Rtools' GCC doesn't have
# `libgcc_eh` due to the compilation settings. So, in order to please the
# compiler, we need to add empty `libgcc_eh` to the library search paths.
#
# For more details, please refer to
# https://github.com/r-windows/rtools-packages/blob/2407b23f1e0925bbb20a4162c963600105236318/mingw-w64-gcc/PKGBUILD#L313-L316
touch ./rust/target/libgcc_mock/libgcc_eh.a
# CARGO_LINKER is provided in Makevars.ucrt for R >= 4.2
if [ "" != "true" ]; then \
    export CARGO_HOME=/c/Users/CCBFC~1.UMP/AppData/Local/Temp/RtmpQJH223/R.INSTALL87823f25c5d/timeless/src/.cargo; \
fi && \
    export CARGO_TARGET_X86_64_PC_WINDOWS_GNU_LINKER="x86_64-w64-mingw32.static.posix-gcc.exe" && \
    export LIBRARY_PATH="${LIBRARY_PATH};/c/Users/CCBFC~1.UMP/AppData/Local/Temp/RtmpQJH223/R.INSTALL87823f25c5d/timeless/src/./rust/target/libgcc_mock" && \
    cargo build --target=x86_64-pc-windows-gnu --lib --release --manifest-path=./rust/Cargo.toml --target-dir ./rust/target
/bin/sh: line 6: cargo: command not found
make: *** [Makevars.win:24: rust/target/x86_64-pc-windows-gnu/release/libtimeless.a] Error 127
ERROR: compilation failed for package 'timeless'
chrisumphlett commented 5 months ago

same issue with remotes::install_github("schochastics/timeless")

chrisumphlett commented 5 months ago

Seems like some kind of issue with a rust based uncompiled package that may not be easily fixed, at least not by someone with my ignorance of how these things work. If I clone this to my machine should I expect it to work by loading with devtools::load_all()? Is rust something I would need to install?

schochastics commented 5 months ago

Yes so when you install from CRAN you install an already compiled version of the package. From github you need to compile the rust code. Here is some help to setup the rust toolchain for R: https://github.com/extendr/libR-sys hope this helps! Otherwise you unfortunately need to wait till the package is on CRAN again

chrisumphlett commented 5 months ago

I don’t want to do that but if I give you some sample data you could run that and I’ll tell you if it’s what I expected/wanted…?I can remove sensitive columns and post the actual dataOn Mar 29, 2024, at 11:04 AM, David Schoch @.***> wrote: Yes so when ou install from CRAN you install an already compiled version of the package. From github you need to compile the rust code. Here is some help to setup the rust toolchain: https://github.com/extendr/libR-sys hope this helps! Otherwise you unfortunately need to wait till the package is on CRAN again

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

schochastics commented 5 months ago

Sure. Post it here or send it via email

chrisumphlett commented 5 months ago
user_dat <- data.frame(  
  user_id = c(1,2,3),  
  created_at = c("2014-04-15T20:32:48Z", "2024-01-01T05:59:36Z", "2023-12-31T20:25:36Z"),  
  last_login_at = c("2024-01-01T05:18:18Z", "2024-01-01T05:59:38Z", "2023-12-31T20:25:37Z")
)  

Then run anytime::anytime() and timeless::chronos() on both of those datetime columns

schochastics commented 5 months ago

looks good

library(anytime)
library(timeless)
user_dat <- data.frame(
    user_id = c(1, 2, 3),
    created_at = c("2014-04-15T20:32:48Z", "2024-01-01T05:59:36Z", "2023-12-31T20:25:36Z"),
    last_login_at = c("2024-01-01T05:18:18Z", "2024-01-01T05:59:38Z", "2023-12-31T20:25:37Z")
)

data.frame(
    created_at = user_dat$created_at,
    created_at_anytime = anytime(user_dat$created_at),
    created_at_chronos = chronos(user_dat$created_at),
    last_login_at = user_dat$last_login_at,
    last_login_at_anytime = anytime(user_dat$last_login_at),
    last_login_at_chronos = chronos(user_dat$last_login_at)
)
#>             created_at  created_at_anytime  created_at_chronos
#> 1 2014-04-15T20:32:48Z 2014-04-15 20:32:48 2014-04-15 20:32:48
#> 2 2024-01-01T05:59:36Z 2024-01-01 05:59:36 2024-01-01 05:59:36
#> 3 2023-12-31T20:25:36Z 2023-12-31 20:25:36 2023-12-31 20:25:36
#>          last_login_at last_login_at_anytime last_login_at_chronos
#> 1 2024-01-01T05:18:18Z   2024-01-01 05:18:18   2024-01-01 05:18:18
#> 2 2024-01-01T05:59:38Z   2024-01-01 05:59:38   2024-01-01 05:59:38
#> 3 2023-12-31T20:25:37Z   2023-12-31 20:25:37   2023-12-31 20:25:37

Created on 2024-03-29 with reprex v2.1.0

I created a binary of the package. Not sure this will work but you can try installing it timeless_0.1.0.9000.tar.gz

chrisumphlett commented 5 months ago

this is great. I like the implementation of tz and to_tz and that both are optional. I tried 2 different ways to install from the binary, neither worked. That's okay. Let me know when it gets to CRAN

> install.packages("C:/Users/c.umphlett/Downloads/tar/timeless_0.1.0.9000.tar.gz", repos = NULL, type = "source")
Installing package into ‘C:/Users/c.umphlett/AppData/Local/R/win-library/4.3’
(as ‘lib’ is unspecified)
* installing *binary* package 'timeless' ...
cp: unknown option -- )
Try '/usr/bin/cp --help' for more information.
ERROR: installing binary package failed
* removing 'C:/Users/c.umphlett/AppData/Local/R/win-library/4.3/timeless'
* restoring previous 'C:/Users/c.umphlett/AppData/Local/R/win-library/4.3/timeless'
Warning in install.packages :
  installation of package ‘C:/Users/c.umphlett/Downloads/tar/timeless_0.1.0.9000.tar.gz’ had non-zero exit status
> ?remotes::install_local()
> remotes::install_local("C:/Users/c.umphlett/Downloads/tar/timeless_0.1.0.9000.tar.gz")
── R CMD build ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
✔  checking for file 'C:\Users\c.umphlett\AppData\Local\Temp\RtmpYZPlHr\remotes5ec847b642a6\timeless/DESCRIPTION' ...
─  preparing 'timeless':
✔  checking DESCRIPTION meta-information ...
   Warning in file(con, "r") :
     cannot open file 'man': No such file or directory
    ERROR
   computing Rd index failed:cannot open the connection
Error: Failed to install 'timeless' from local:
  ! System command 'Rcmd.exe' failed
schochastics commented 5 months ago

ok great. I try to remember to let you know here, otherwise check here under windows binaries when it says 0.2.0.

I do the submission today

schochastics commented 5 months ago

@chrisumphlett the new version is on CRAN now

chrisumphlett commented 5 months ago

Great. In my first test, anytime takes 37 seconds, timeless::chronos() takes only one second! Now I have a lot of places to update :)