ncss-tech / soilDB

soilDB: Simplified Access to National Cooperative Soil Survey Databases
http://ncss-tech.github.io/soilDB/
GNU General Public License v3.0
81 stars 20 forks source link

`waterDayYear()` needs fixing (again) for locale #268

Closed brownag closed 1 year ago

brownag commented 2 years ago

waterDayYear() failing again (https://github.com/ncss-tech/soilDB/pull/223, https://github.com/ncss-tech/soilDB/pull/237) on CRAN for several R-devel architectures.

This may not be specifically related to R-devel changes, as our regular tests pass on GHA, but rather likely related to the locale the tests are being run in combined with recent R-devel changes to date/time related functions

  ── Failure (test-waterDayYear.R:15:3): works as expected ───────────────────────
  res.lr$wd not equal to 153L.
  1/1 mismatches
  [1] 152 - 153 == -1
  ── Failure (test-waterDayYear.R:16:3): works as expected ───────────────────────
  res$wd not equal to 152L.
  1/1 mismatches
  [1] 151 - 152 == -1
  ── Failure (test-waterDayYear.R:23:3): works as expected ───────────────────────
  res$wd not equal to 1.
  1/1 mismatches
  [1] 0 - 1 == -1
  ── Failure (test-waterDayYear.R:30:3): works as expected ───────────────────────
  res$wd not equal to 365.
  1/1 mismatches
  [1] 364 - 365 == -1
  ── Failure (test-waterDayYear.R:38:3): works as expected ───────────────────────
  res$wd not equal to c(50, 140).
  2/2 mismatches (average diff: 1)
  [1] 49 - 50 == -1
  [2] 139 - 140 == -1

R-devel related changes:

  • The as.POSIXlt(<POSIXlt>) and as.POSIXct(<POSIXct>) default methods now do obey their tz argument, also in this case.

  • as.POSIXlt(<Date>) now does apply a tz (timezone) argument, as does as.POSIXct(); partly suggested by Roland Fuß on the R-devel mailing list.

  • as.Date.POSIXct(., tz) now treats several ‘tz’ values, notably ‘"GMT"’ as equivalent to ‘"UTC"’, proposed and improved by Michael Chirico and Joshua Ulrich in PR#17674.

  • as.POSIXct(<numeric>) and as.POSIXlt(.) (without specifying ‘origin’) now work. So does as.Date(<numeric>), too.

  • As ‘"POSIXlt"’ objects may be “partially filled” and their list components meant to be recycled, ‘length()’ now is the length of the longest component.

brownag commented 1 year ago

Just got our two-weeks notice from CRAN that this needs to be fixed.

dylanbeaudette commented 1 year ago

Dang. I don't have time to figure this out. Any suggestions other than removing the test?

brownag commented 1 year ago

This has now been fixed: tested locally on R 4.2.1 and R-devel, and run through winbuilder to confirm it is without error.

As I expected it was an issue with the specific timezone that these checks were being run. There are new tests that check the related problems locally... but this is nearly 600 timezones--so now after having verified they work on CRAN/R-devel I skip them. There are several where the time difference calculation will be plus or minus 1 hour depending on the start/end points... so that also needed to be accommodated in the conversion of difftime to integer.

I made a couple other changes as well: 1) better support for alternate WY end date 2) changed default time zone to "UTC".

I think standardizing on UTC for these (and related) functions will make it easier to do localization if/when, for example, we add timezone metadata to the Henry DB or make use of it for SCAN data integration.

dylanbeaudette commented 1 year ago

Thanks, this is a big help. Switching to UTC for computation was a clever idea. This is a good reminder to get the 'tz' column added to Henry Mount DB.