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

grains:total impacted by rounding errors #106

Open
Gamecock opened this issue Aug 19, 2018 · 16 comments
Open

grains:total impacted by rounding errors #106

Gamecock opened this issue Aug 19, 2018 · 16 comments

Comments

@Gamecock
Copy link

Becuase total is a float it is actually comparing:
`Error: Test failed: 'returns the total number of square on the board'

  • total() not equal to 1.844674e+19. If you use option(digits = 22) you can see:* total() not equal to 18446744073709551616.`

Test was supposed to be:
expect_equal(total(), 18446744073709551615)

See thread for more discussion. Do you want to teach bigInts and importing libraries this early in the track?

@katrinleinweber
Copy link
Contributor

Hello @Gamecock,

could you please post your sessionInfo() output? I'm not sure why you are seeing this different behavior. Here's mine:

R version 3.5.0 (2018-04-23)
Platform: x86_64-apple-darwin15.6.0 (64-bit)
Running under: macOS High Sierra 10.13.6

Matrix products: default
BLAS: /System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/Versions/A/libBLAS.dylib
LAPACK: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRlapack.dylib

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] testthat_2.0.0

loaded via a namespace (and not attached):
[1] compiler_3.5.0 magrittr_1.5   R6_2.2.2       tools_3.5.0    withr_2.1.2   
[6] yaml_2.2.0     rlang_0.2.2 

@jonmcalder Can you reproduce the test fail that Mike reports?

@Gamecock
Copy link
Author

Gamecock commented Sep 5, 2018

sessionInfo()
R version 3.4.3 (2017-11-30)
Platform: x86_64-apple-darwin15.6.0 (64-bit)
Running under: macOS Sierra 10.12.6

Matrix products: default
BLAS: /System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/Versions/A/libBLAS.dylib
LAPACK: /Library/Frameworks/R.framework/Versions/3.4/Resources/lib/libRlapack.dylib

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats graphics grDevices utils datasets methods base

loaded via a namespace (and not attached):
[1] compiler_3.4.3 tools_3.4.3 yaml_2.1.18

@katrinleinweber
Copy link
Contributor

Thank you. The easiest possible culprit to exclude is your R v3.4.3 (although there should have been other users noticing this problem then). Can you try upgrading to 3.5.x from one of the mirrors?

@Gamecock
Copy link
Author

Gamecock commented Sep 5, 2018

I'll try, but isn't the expected result that if you try to compare two large numbers beyond the size of an Integer you lose precision?

@Gamecock
Copy link
Author

Gamecock commented Sep 5, 2018

sessionInfo()
R version 3.5.1 (2018-07-02)
Platform: x86_64-apple-darwin15.6.0 (64-bit)
Running under: macOS Sierra 10.12.6

Matrix products: default
BLAS: /System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/Versions/A/libBLAS.dylib
LAPACK: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRlapack.dylib

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats graphics grDevices utils datasets methods base

loaded via a namespace (and not attached):
[1] compiler_3.5.1 tools_3.5.1

@Gamecock
Copy link
Author

Gamecock commented Sep 5, 2018

Same behavior, this should fail.
library(testthat)

expect_equal(18446744073709551616, 18446744073709551615)

The solution is:
library("gmp")
library("testthat")
expect_equal(as.bigz(2)^64, as.bigz(2)^64-1)
Error: as.bigz(2)^64 not equal to as.bigz(2)^64 - 1.
'target'(bigz) and 'current' differ

@katrinleinweber
Copy link
Contributor

… isn't the expected result that if you try to compare two large numbers beyond the size of an Integer you lose precision?

Ah, no I see what you mean. identical(18446744073709551616, 18446744073709551615) yields TRUE here, as well, but it shouldn't. Sorry for the version detour!

Which "thread for more discussion" do you mean?

@jonmcalder, @jkanche & @amoradell: What do you think about teach[ing] bigInts and importing libraries this early in the track?

@jonmcalder
Copy link
Member

Sorry for the delayed response - I'm travelling at the moment so haven't got much capacity for keeping tabs on issues here.

Am I correct in saying that the issue @Gamecock is raising here is the same as referred to in #84?

Essentially base R doesn't provide support for large integers and we didn't have consensus previously on how to deal with this.

My sense (revisiting it again now), is that the issue of precision is in some ways fundamental to the problem and we should have a way of supporting more precise testing for the exercise.

Two potential solutions come to mind:

  • include a hint to use a particular R package to provide support for big integers and extend the exercise with optional tests (i.e. commented out by default) that implement more precise matching using the relevant type from this package (making this more advanced solution optional should then mean we can keep this exercise early in the track)
  • alternatively we could move the exercise later in the track, have a hard requirement to use a specific library and include the relevant test as a "required" test to pass for the exercise

@Gamecock
Copy link
Author

Gamecock commented Sep 7, 2018

@jonmcalder that is the correct issue, I didn't think to look through closed issues. I have sufficient skills to do the work, but don't have enough experience with r to determine the correct path.

@katrinleinweber
Copy link
Contributor

+1 for the 2nd option.

@jonmcalder
Copy link
Member

Ok cool. I'm happy with the second option too. @Gamecock are you happy to get started on a PR and then raise any further questions on implementation details with @katrinleinweber or me?

@Gamecock
Copy link
Author

Gamecock commented Sep 7, 2018 via email

@amoradell
Copy link

I approve second option.

I suppose that core exercices and easy ones should be made with R base package.

@beansprout

This comment has been minimized.

@katrinleinweber

This comment has been minimized.

@beansprout

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants