Skip to content
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

sunrise_time does not update time value for requested time zone #7

Closed
putmanlab opened this issue Jul 15, 2022 · 6 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@putmanlab
Copy link

It seems sunrise_time is not correct for non-UTC times with recent versions of lubridate or R, as shown in the User Guide.

library(lubridate)
library(photobiology)

my.geocode <- data.frame(lat = 60.16, lon = 24.93, address = "Helsinki")
my.datetime <- as_datetime("2022-07-10 12:00:00", tz="EET")

sunrise_time(my.datetime, geocode = my.geocode, tz = "UTC")
[1] "2022-07-10 01:13:46 UTC"

sunrise_time(my.datetime, geocode = my.geocode, tz = "EET")
[1] "2022-07-10 01:13:46 EEST"

> sessionInfo()
R version 4.2.0 (2022-04-22)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 20.04.4 LTS

Matrix products: default
BLAS:   /usr/lib/x86_64-linux-gnu/openblas-pthread/libblas.so.3
LAPACK: /usr/lib/x86_64-linux-gnu/openblas-pthread/liblapack.so.3

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C               LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8    
 [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8    LC_PAPER=en_US.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C             LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] photobiology_0.10.2 tibble_3.1.7        lubridate_1.8.0    

loaded via a namespace (and not attached):
 [1] compiler_4.2.0    pillar_1.7.0      prettyunits_1.1.1 remotes_2.4.2     tools_4.2.0       testthat_3.1.4    pkgbuild_1.3.1   
 [8] pkgload_1.2.4     memoise_2.0.1     lifecycle_1.0.1   pkgconfig_2.0.3   rlang_1.0.2       DBI_1.1.3         cli_3.3.0        
[15] fastmap_1.1.0     withr_2.5.0       dplyr_1.0.9       generics_0.1.2    desc_1.4.1        fs_1.5.2          vctrs_0.4.1      
[22] devtools_2.4.3    rprojroot_2.0.3   tidyselect_1.1.2  glue_1.6.2        R6_2.5.1          processx_3.6.1    fansi_1.0.3      
[29] sessioninfo_1.2.2 callr_3.7.0       purrr_0.3.4       magrittr_2.0.3    ps_1.7.1          ellipsis_0.3.2    usethis_2.1.6    
[36] assertthat_0.2.1  utf8_1.2.2        cachem_1.0.6      crayon_1.5.1      brio_1.1.3   

However, regressing to earlier versions of lubridate and R but using the same photobiology version results in the correct time.

library(lubridate)
library(photobiology)

my.geocode <- data.frame(lat = 60.16, lon = 24.93, address = "Helsinki")
my.datetime <- as_datetime("2022-07-10 12:00:00", tz="EET")

sunrise_time(my.datetime, geocode = my.geocode, tz = "UTC")
[1] "2022-07-10 01:13:46 UTC"

sunrise_time(my.datetime, geocode = my.geocode, tz = "EET")
[1] "2022-07-10 04:13:46 EEST"

sessionInfo()
R version 3.6.3 (2020-02-29)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Debian GNU/Linux 10 (buster)

Matrix products: default
BLAS/LAPACK: /usr/lib/x86_64-linux-gnu/libopenblasp-r0.3.5.so

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C               LC_TIME=en_US.UTF-8       
 [4] LC_COLLATE=en_US.UTF-8     LC_MONETARY=en_US.UTF-8    LC_MESSAGES=C             
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                  LC_ADDRESS=C              
[10] LC_TELEPHONE=C             LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] photobiology_0.10.2 tibble_3.0.1        lubridate_1.7.8    

loaded via a namespace (and not attached):
 [1] Rcpp_1.0.4.6      compiler_3.6.3    pillar_1.4.3      prettyunits_1.1.1 remotes_2.1.1    
 [6] tools_3.6.3       testthat_2.3.2    digest_0.6.25     pkgbuild_1.0.6    pkgload_1.0.2    
[11] memoise_1.1.0     lifecycle_0.2.0   pkgconfig_2.0.3   rlang_0.4.5       cli_2.0.2        
[16] rstudioapi_0.11   withr_2.2.0       dplyr_0.8.5       generics_0.0.2    desc_1.2.0       
[21] fs_1.4.1          vctrs_0.2.4       devtools_2.3.0    rprojroot_1.3-2   tidyselect_1.0.0 
[26] glue_1.4.0        R6_2.4.1          processx_3.4.2    fansi_0.4.1       sessioninfo_1.1.1
[31] purrr_0.3.4       callr_3.4.3       magrittr_1.5      backports_1.1.6   ps_1.3.2         
[36] ellipsis_0.3.0    usethis_1.6.0     assertthat_0.2.1  crayon_1.3.4  
@aphalo
Copy link
Owner

aphalo commented Jul 16, 2022

Many thanks for this report!
I will investigate as soon as possible what has changed in 'lubridate', and if the bug is in 'photobiology' or 'lubridate'. I suspect the problem may be caused by a work-around I have in 'photobiology' for a situation where the tz was not set correcly by 'lubridate'.

@aphalo aphalo self-assigned this Jul 16, 2022
@aphalo aphalo added the bug Something isn't working label Jul 16, 2022
@aphalo aphalo added this to the v0.11.0 milestone Jul 16, 2022
@aphalo
Copy link
Owner

aphalo commented Jul 16, 2022

I suspect the problem has to do with the change in 'lubridate' from "UTC" as default time zone to "" as default time zone in functions like "now()" and conversion functions like "ymd()".

@aphalo
Copy link
Owner

aphalo commented Jul 17, 2022

This guess was wrong. The problem stems at least in part from the use of lubridate::today() as default for date in sunrise_time(), sunset_time() and noon_time() in 'photobiology' as today() returns a Date instead of a POSIXct and thus lacks a time zone setting. Another problem is that lubridate::floor_date() converts POSIXct objects to Date, thus dropping the time zone information. I have started the revision to the code but it is not yet working when the argument passed to date has length > 1.

aphalo added a commit that referenced this issue Jul 18, 2022
'lubridate' 1.8.0 broke handling of time zones in sunrise_time(). Working now for time vectors or Date vectors of length 1.  Issue #7
aphalo added a commit that referenced this issue Jul 18, 2022
Pending testing with earlier versions, now these new versions of lubridate and tibble are the minimum supported. This version fixes the bug reported in issue #7 for sunrise_time(), affecting also day_night(), sunset_time() and noon_time().
@aphalo
Copy link
Owner

aphalo commented Jul 18, 2022

Added unit tests and tested with 'lubridate' 1.7.8, 1.7.9.2 and 1.8.0. Tested with 'tibble' 3.1.0 and 3.1.7. All under R 4.2.1. The previously buggy functions now return the correct values in all cases, independently of input being a Date or POSIXct object. For Date arguments passed to date the default is tz = "UTC". The default for date is now lubridate function now() instead of today(), now called with tz = "UTC". Currently tz is used to determine to which data an instant in time belongs.

@aphalo aphalo closed this as completed Jul 18, 2022
@putmanlab
Copy link
Author

It is fixed for me. Thanks for the rapid response, and many thanks for developing a very useful package!

@aphalo
Copy link
Owner

aphalo commented Jul 20, 2022

You are welcome! Once again thanks for reporting the problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants