units::set_units cannot work with class "difftime"

[istfer]

Bug Description

Recent udunits2 to units change introduced a bug to one of our common met process pipeline. This met2CF.csv line is now throwing an error:

Error in UseMethod("set_units") : 
  no applicable method for 'set_units' applied to an object of class "difftime"

To Reproduce

Try the demo run or see:

days_since_1700 <- as.POSIXct(c("2003-01-01 00:00:00 UTC", "2003-01-01 00:30:00 UTC")) - lubridate::ymd_hm("1700-01-01 00:00")
as.numeric(mean(PEcAn.utils::ud_convert(diff(days_since_1700), "d", "s")))

Proposed solution

We can change class from difftime to numeric before conversion:

mean(PEcAn.utils::ud_convert(as.numeric(diff(days_since_1700)), "d", "s"))

or any other solutions are welcome.

[nanu1605]

Hi @istfer, If we replace as.numeric(mean(PEcAn.utils::ud_convert(diff(days_since_1700), "d", "s"))) with as.numeric(mean(PEcAn.utils::ud_convert(as.numeric(diff(days_since_1700)), "d", "s"))) it works. I don't know if this is the right way to do it but the output I am getting after running this code is 1800.

days_since_1700 <- as.POSIXct(c("2003-01-01 00:00:00 UTC", "2003-01-01 00:30:00 UTC")) - lubridate::ymd_hm("1700-01-01 00:00")
as.numeric(mean(ud_convert(as.numeric(diff(days_since_1700)), "d", "s")))

[istfer]

Hi @nanu1605 yes I think that would solve it, but also maybe the outermost as.numeric is unnecessary in that case as the inner evaluation will return numeric class object, i.e. we can just do mean(PEcAn.utils::ud_convert(as.numeric(diff(days_since_1700)), "d", "s"))

Alternatively, we could make sure x is numeric (or one of the recognized classes) inside the ud_convert function itself, but I'm not sure if we want to enforce it there as it could lead to unwanted bugs (e.g. if someone passes a factor by mistake current implementation will throw an error, but if we force it to be as.numeric it could go unnoticed. On the other hand, if someone wants to use this function they probably meant to have a numeric value not a factor in the first place). Well, function documentation does say it expects x to be numeric, so maybe we can add a check and throw a more informative warning/error other than Error in UseMethod("set_units") (although finding the culprit was also not difficult with this particular error message)

What do others think? For the sake of being practical, you could just go with the change in the met2CF.csv (while you're at it you could check if there are other similar occurrences in the codebase too) and PR the fix.

[nanu1605]

Hi @istfer have you updated the met2CF.csv and changed as.numeric(mean(PEcAn.utils::ud_convert(diff(days_since_1700), "d", "s"))) to mean(PEcAn.utils::ud_convert(as.numeric(diff(days_since_1700)), "d", "s")) ?

[infotroph]

I think this definitely calls for changes inside ud_convert rather than in the code that calls it. This is a case that the udunits2 conversion did more or less handle, albeit with a buglet of its own:

> dt <- as.POSIXct("2021-01-01 13:00:00") - as.POSIXct("2021-02-02 12:00:00")
> dt
Time difference of 1.041667 days
> udunits2::ud.convert(dt, "days", "hours")
Time difference of 25 days

Notice that 25 is the correct number of hours, but the resulting difftime object thinks it's 25 days. This happens because ud.convert constructs a new numeric vector containing the converted results and then blindly copies all attributes from the input object, including the now-inaccurate units attribute. I think we definitely do NOT want ud_convert to preserve that behavior.

One option would be for PEcAn.utils::ud_convert to detect when x inherits from difftime and handle it using base R's time conversion functions instead of the udunits library, something like...

if (inherits(x, "difftime") {
  stopifnot(units(x) == u1)
  units(x) <- u2 #NB performs conversion, doesn't just relabel
  return(x)
}

...but I don't know how closely the difftime and udunits results will align. Another option would be to pass the values as numeric to udunits, but detect when x inherits from difftime and do some extra work to reassemble a difftime object with the correct units after conversion.

[nanu1605]

@infotroph I think adding the above snippet is a great idea for now. Should I open a PR for it?

[infotroph]

@nanu1605 On further consideration I think we want to have the conversion be done inside the udunits library not units<-, so the snippet above needs updating. The process should be:

• detect whether x already has units attached to it
• if so,
  • make sure u1 is the same as the units x specifies (note that "the same" should probably allow for units that simplify to identical, e.g. "sec" vs "seconds" or "g/kg" vs "(10g)/(10kg)"
  • coerce x to numeric
• convert x to the new units as already implemented
• If x had units when it came in, coerce the converted result to the same class as x with units u2
• return the result

[nanu1605]

@infotroph I changed the function a little. Its working fine, but I am not sure it's the right method or not

ud_convert <- function(x, u1, u2) {
  y <- as.numeric(x)
  stopifnot(units::ud_are_convertible(u1, u2))
  x1 <- units::set_units(y, value = u1, mode = "standard")
  x2 <- units::set_units(x1, value = u2, mode = "standard")
  if(class(x)!="numeric"){
    x2
  }
  else(units::drop_units(x2))
}

[Aariq]

I think class difftime already has units and it needs to be coerced to a units object with as_units() then the units can be changed with set_units().

library(units)
#> udunits database from /usr/local/share/udunits/udunits2.xml
library(lubridate)
#> 
#> Attaching package: 'lubridate'
#> The following objects are masked from 'package:base':
#> 
#>     date, intersect, setdiff, union
days_since_1700 <- as.POSIXct(c("2003-01-01 00:00:00 UTC", "2003-01-01 00:30:00 UTC")) - lubridate::ymd_hm("1700-01-01 00:00")

class(days_since_1700)
#> [1] "difftime"
units(days_since_1700)
#> [1] "days"

days_since_1700 |> as_units() |> set_units("seconds")
#> Units: [s]
#> [1] 9561733200 9561735000

^{Created on 2023-05-16 with reprex v2.0.2}

I think the safest way to go would be with a modified version of what @infotroph suggested here: #3012 (comment)

I'll make a quick PR