hhoeflin / hdf5r

Other
80 stars 23 forks source link

`devtools::test` failed #178

Closed dipterix closed 3 years ago

dipterix commented 3 years ago

I tried devtools::test() on github fork. It showed an error:

Failure (test-64bit_support.R:88:5): Datatype conversion with 64bit
suppressWarnings(truncateVec(dbl_vec_int64_na, 0, LLONG_MAX)) not equal to res_dbl_uint64_na$output.
'is.NA' value mismatch: 3 in current 2 in target
hhoeflin commented 3 years ago

Yeah, this has been a bit of a problem lately. the test is supposed to check behaviour with NAs, but there seems to be inconsistencies between platforms. At least this is the only way I see it failing on some but not others. As I don't have an M1 I can't debug this.

dipterix commented 3 years ago

Please let me know if I can help. I can fork and test on my machine.

Also I requested a PR #177 so that you can have github action to check the package for you on osx. A script to install (without brew) is also included in that PR. This might be helpful when brew arch is different from R (for example, ARM brew with x86 R installed, users can compile x86 HDF5 by themselves without having two brews in the same system)

hhoeflin commented 3 years ago

Thanks for this - I will merge it. And yes, please debug it, that would be great. Am thankful for help. Personally I have not used this package in years, so am doing this in my spare time as much as I can.

dipterix commented 3 years ago

@hhoeflin I don't know how to fix, but I can help you debug, and provide results on M1.

Here's the issue

https://github.com/hhoeflin/hdf5r/blob/5c9074f0d9a19150548a69da01ea342e76db1018/tests/testthat/test-64bit_support.R#L88

image

2^63-1, i.e. LLONG_MAX is NA in hdf5r, but in bit64, NA should be 2^63?

All other tests passed.

dipterix commented 3 years ago

More information:

Bit information: 9223372036854775807 vs int64 NA

> bit64::as.bitstring(dbl_vec_int64_na[18])
[1] "0111111111111111111111111111111111111111111111111111111111111111"

> bit64::as.bitstring(bit64::NA_integer64_)
[1] "1000000000000000000000000000000000000000000000000000000000000000"
dipterix commented 3 years ago

The problem is here:

https://github.com/hhoeflin/hdf5r/blob/5c9074f0d9a19150548a69da01ea342e76db1018/src/convert.c#L278

When Rval=9223372036854775807 (REALSXP), converting to uint64_t result in 9223372036854775808

This is so weird... I was wondering if this is R's fault :/

Wrote a simple C to test it:

SEXP print_bit(SEXP obj){
  uint64_t tmp = *REAL0(obj);
  printf("%llu ", tmp);
  return(R_NilValue);
}
> bit64::as.integer64(dbl_vec[18])
integer64
[1] 9223372036854775807
> print_bit(dbl_vec[18])
9223372036854775808 NULL
dipterix commented 3 years ago

It seems 2^63 produces different results on M1 vs x86 when converting to int64_t.

This little difference cause bit64 package to interpret 2^63 as 9223372036854775807 instead of NA on M1 mac, which I think is their flaw.

I've posted this issue on bit64 package.

https://github.com/truecluster/bit64/issues/19

Your package seems fine. Small changes on the test might be desired. I'll create a PR soon.

dipterix commented 3 years ago

Asked help from r-devel, their reply seems confirming my guess: https://stat.ethz.ch/pipermail/r-devel/2021-August/080986.html

bit64 is handling an undefined conversion, and hdf5r is doing right.

hhoeflin commented 3 years ago

Great - thanks for following up on this. If this is truly undefined, then yes I think bit64 needs to handle this as per your pull request. Over the weekend I have some time and will review and merge all this. Thanks for all your work!

On Thu, Aug 12, 2021 at 1:47 PM Dipterix Wang @.***> wrote:

Asked help from r-devel, their reply seems confirming my guess: https://stat.ethz.ch/pipermail/r-devel/2021-August/080986.html

bit64 is handling an undefined conversion, and hdf5r is doing right.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/hhoeflin/hdf5r/issues/178#issuecomment-897571826, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASGMYU7BUPCTEOLM7Z5Z2TT4OYD7ANCNFSM5B6XFTEQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .