Update var.h #320
Update var.h #320
Conversation
Variance: Change from the unstable one-pass formula back to the stable two-pass formula. Details: RcppCore#223
| const double | ||
| average = mean(object).get(), | ||
| sum_squared_deviations = sum( pow(object - average, 2.0) ).get(); | ||
| return sum_squared_deviations / (object.size() - 1); | ||
| } |
There was a problem hiding this comment.
👍
Could you do the same for the complex case (next few lines) and also add ChangeLog entry in the PR?
There was a problem hiding this comment.
Just added a comment, it's indeed next in line in .plan :-)
There was a problem hiding this comment.
Looks like this might have introduced a regression for IntegerVector, investigating.
There was a problem hiding this comment.
Found the reason.
For IntegerVector temporary object - average has type Rcpp::sugar::Minus_Vector_Primitive<13, true, Rcpp::Vector<13, Rcpp::PreserveStorage>>.
In contrast, for NumericVector temporary object - average has type Rcpp::sugar::Minus_Vector_Primitive<14, true, Rcpp::Vector<14, Rcpp::PreserveStorage>>.
/* Magic numbers corresponding to:
13 INTSXP integer vectors
14 REALSXP numeric vectors
http://cran.r-project.org/doc/manuals/r-release/R-ints.pdf */
This results in invalid deviations calculation (thus resulting in invalid sum_squared_deviations).
Rewriting.
Edit: done, complex included; if everything seems in order can also proceed with the ChangeLog.
Make this safer for IntegerVector
(Feature) Add tests for complex variance computation (unimplemented in R -- thus, comparing against known synthetic cases). (Refactoring) Introduce names in test cases replacing magic numbers (improves Travis CI reports readability), apply DRY to test data, robustify.
Implement variance for complex numbers in accordance with its definition. Reference: https://en.wikipedia.org/wiki/Variance#Generalizations
|
I think this one is good to go (though I'd like a Any seconds, @kevinushey or @jjallaire ? |
|
@eddelbuettel: sounds great; done & done! :-) |
|
Sorry: I fixed this in 384e7e4 but I hope that doesn't create a merge conflict for you now. |
|
Right! Can I fix this on my side? |
|
I would think so by re-syncing your fork with what I have now. |
|
Let's hope this works :-) |
|
Looks good! While I usually wait for either/both of @kevinushey and @jjallaire to nod, I think this one is good to go -- thanks for the contribution! |
|
:-) |
Variance: Change from the unstable formula back to the stable (two-pass) formula.
Details: #223