vec_cast.units.units() can fail if one underlying vector is integer

[hughjonesd]

Example:

x <- set_units(1:10, cm)
br <- set_units(2:4, `in`)
vctrs::vec_cast_common(x, br)
Error:
! Can't convert from `..2` <double> to <integer> due to loss of precision.
• Locations: 1, 2, 3

Similarly:

x_m <- set_units(1:10, m)
br_ft <- set_units(2:6, ft)
vctrs::vec_cast_common(x_m, br_ft)
# fails with the same error message

By contrast:

x <- set_units(1:10, cm)
br_ft2 <- set_units(2:4/12, ft)
vctrs::vec_cast_common(x, br_ft2)

works. So does:

> x_in <- set_units(1:10, `in`)
> vctrs::vec_cast_common(x_in, br)

and:

x_dbl <- set_units(as.double(1:10), cm)
vctrs::vec_cast_common(x_dbl, br)

Lastly, you avoid the bug if the double happens to evaluate to integer values:

x <- set_units(1:10, cm)
br2 <- set_units(2:4 / 2.54, `in`)
vctrs::vec_cast_common(x, br2)
# works

The issue seems to be that units::vec_cast.units.units() recalls vec_cast with the underlying bare data. But if one of these is integer, and the other is double and is not integer-valued (e.g. because it's been converted), that fails.

This may make sense for reasons that I don't understand. It was surprising to me, though.

I think the issue is that vctrs correctly wants to fail if it has to cast a non-integer-valued double to an integer. But in the units case, we can be sure that the integer values are "really" doubles, i.e. they are on the real number line (... right?) If so, maybe a fix would be to add to_bare <- as.Double(to_bare) in vec_cast.units.units().

Package versions: units 0.8.0, vctrs 0.4.1, R 4.2.0.

[Enchufa2]

Thanks for the analysis. The question here is why are we recalling the method on the bare data. Is there any corner case I don't see in which this is required, @lionel- ?

units/R/tidyverse.R

Line 72 in 07b2606

out = vctrs::vec_cast(out_bare, to_bare, ..., x_arg = x_arg, to_arg = to_arg)

Can't the method finish 4 lines before, when the conversion was already made?

[lionel-]

yup it could. I thought it'd be a good thing to benefit from vctrs casting rules for bare types but maybe not?

[Enchufa2]

For units, we care about the mode (numeric) but not about the storage mode. From what I can see, ptype2 is about the mode, and cast takes into account the storage mode. Is that right? So, unless there's some benefit I don't see, I'll just drop that part and adjust the tests accordingly.

[lionel-]

hmm... Actually it's important for vctrs that after a cast from x to y, the cast result has the same storage mode as y.

Alternatively, you could provide a proxy method for your class that always uses the same storage mode, that would work too.

[Enchufa2]

Ok. The proxy thing is too much trouble I think. How about forcing the storage mode of the destination? See 85c9785

[hughjonesd]

Can confirm this fixes my underlying bug.

[lionel-]

If you have an identity proxy, the storage mode of the return value of a cast method must match the storage mode of to. So if to is not stored as a double, this cast method will cause internal errors in some cases.

[Enchufa2]

Can you think of an example of such errors, so that we can use it as a test case?

[lionel-]

If x has integer storage and y has double storage, and the cast method doesn't normalise that, and there is an identity proxy, I believe vec_c(x, y) should fail.

[Enchufa2]

It works fine with the commit above:

x = units::set_units(1:3, "cm")
y = units::set_units(4.0, "m")
storage.mode(x)
#> [1] "integer"
storage.mode(y)
#> [1] "double"
vctrs::vec_c(x, y)
#> Units: [cm]
#> [1]   1   2   3 400
vctrs::vec_c(y, x)
#> Units: [m]
#> [1] 4.00 0.01 0.02 0.03

[lionel-]

oh yeah, because the common type of int + dbl == dbl, and your cast method always converts to dbl.

But here is an example where it breaks:

vctrs::vec_assign(x, 2, y)
#> Error in `vctrs::vec_assign()`:
#> ! `proxy` of type `integer` incompatible with `value` proxy of type `double`.
#> ℹ In file slice-assign.c at line 156.
#> ℹ This is an internal error in the vctrs package, please report it to the package authors.
#>     ▆
#>  1. ├─vctrs::vec_assign(x, 2, y)
#>  2. └─rlang:::stop_internal_c_lib(...)
#>  3.   └─rlang::abort(message, call = call, .internal = TRUE, .frame = frame) at rlang/R/utils.R:212:2

In general, not respecting the storage mode in a vctrs cast method is UB.

[hughjonesd]

I take it back; my bug is still there using units 0.8.1 (and vctrs 0.6.2).

x <- set_units(1:10, cm)
br <- set_units(2:4, `in`)
vctrs::vec_cast_common(x, br)
Error:
! Can't convert from `..2` <double> to <integer> due to loss of precision.
• Locations: 1, 2, 3

[Enchufa2]

Yes, because I never merged the commit above per Lionel's last comment, and I didn't have the time to further look into this, apologies.

[hughjonesd]

So is there a reason not to just convert manually to double? My thoughts, FWIW:

• this error only bites the user sometimes, which makes it risky
• presumably if two units are mutually convertible, they can always be thought of as both double (or both integer, maybe - I don't actually know if there are any integer-only units)