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

devtools::test failed #178

Closed
dipterix opened this issue Aug 11, 2021 · 9 comments
Closed

devtools::test failed #178

dipterix opened this issue Aug 11, 2021 · 9 comments

Comments

@dipterix
Copy link

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
Copy link
Owner

hhoeflin commented Aug 11, 2021

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

dipterix commented Aug 11, 2021

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

Here's the issue

expect_equal(suppressWarnings(truncateVec(dbl_vec_int64_na, 0, LLONG_MAX)), res_dbl_uint64_na$output)

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
Copy link
Author

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
Copy link
Author

dipterix commented Aug 11, 2021

The problem is here:

uint64_t* uint64_ptr = (uint64_t *) REAL(Rval);

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
Copy link
Author

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

  • on x86, bit converting 2^63 to int64_t is -9223372036854775808
  • on M1 mac, bit converting 2^63 to int64_t is 9223372036854775807

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.

r-lib/bit64#19

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

@dipterix
Copy link
Author

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
Copy link
Owner

hhoeflin commented Aug 12, 2021 via email

hhoeflin pushed a commit that referenced this issue Aug 14, 2021
* added github actions to check the repo

* added build ignore for gh action checks; set system path to include h5cc

* minor fix for non-osx

* updated to HDF5 1.12.1

* See #178 and r-lib/bit64#19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants