-
-
Notifications
You must be signed in to change notification settings - Fork 208
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
Sugar median with unit tests #425
Conversation
@@ -0,0 +1,297 @@ | |||
// -*- mode: C++; c-indent-level: 4; c-basic-offset: 4; tab-width: 8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a much newer version of this line but then I never said anything official about it...
This looks good. I had in the back of my mind a though that R's So this is probably good too but one or two seconds would be nice. |
Okay, that sounds good. In the meantime I will make the appropriate changes regarding your comments above. Is this the newer version of the formatting line you referred to:
|
Yep. Hard tabs are evil, which we learned eventually. But I leave commits touching every single file to @kevinushey :) -- in short, bulk changes aren't worth it either. There isn't too much about code style you don't already know or do. Four spaces is better than too, indentation pretty much like R Core and other sensible people do etc pp. No biggies. |
Agreed on the spaces vs. tabs - it was just sloppy copy & paste on my part from another sugar file, and being a |
FWIW we now use |
|
||
public: | ||
Median(const VECTOR& xx) | ||
: x(Rcpp::clone(xx)) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm -- we clone the vector here because the std::
algorithm we use will modify the passed vector?
Maybe we can also have a But if we really want to have a consistent coding style, we might think about cpplint from Google. |
Re Alternatively, and there clang-format but all of that is a bit of the heavy-handed side. |
My bad for the close and re-open. Hit the wrong button. |
Other than some minor comments this looks good to me. |
@kevinushey Thanks for the feedback - I do believe it is necessary to call #include <Rcpp.h>
// [[Rcpp::export]]
double median_dbl(Rcpp::NumericVector x, bool na_rm = false) {
return Rcpp::median(x, na_rm);
}
// [[Rcpp::export]]
double median_int(Rcpp::IntegerVector x, bool na_rm = false) {
return Rcpp::median(x, na_rm);
}
// [[Rcpp::export]]
Rcomplex median_cx(Rcpp::ComplexVector x, bool na_rm = false) {
return Rcpp::median(x, na_rm);
}
// [[Rcpp::export]]
Rcpp::String median_ch(Rcpp::CharacterVector x, bool na_rm = false) {
return Rcpp::median(x, na_rm);
}
/*** R
set.seed(123)
(xx <- rnorm(5))
#[1] -0.56047565 -0.23017749 1.55870831 0.07050839 0.12928774
median_dbl(xx)
#[1] 0.07050839
xx
#[1] -0.56047565 -0.23017749 0.07050839 0.12928774 1.55870831
set.seed(123)
(xx <- as.integer(rpois(5, 20)))
#[1] 17 25 12 20 27
median_int(xx)
#[1] 20
xx
#[1] 17 12 20 25 27
set.seed(123)
(xx <- rnorm(5) + 2i)
#[1] -0.560476+2i -0.230177+2i 1.558708+2i 0.070508+2i 0.129288+2i
median_cx(xx)
#[1] 0.070508+2i
xx
#[1] -0.560476+2i -0.230177+2i 0.070508+2i 0.129288+2i 1.558708+2i
set.seed(123)
(xx <- sample(letters, 5))
#[1] "h" "t" "j" "u" "w"
median_ch(xx)
#[1] "t"
xx
##[1] "h" "j" "t" "u" "w"
*/ As for the other points, I'll set I have not used / am not aware of anything analogous to |
Time to pull this one in! |
Sugar median with unit tests
👍 |
No, thumbs up to you -- that was really well done (but I was a little tied up yesterday and the day before). |
As discussed in #424, this adds a sugar function
Rcpp::median
with corresponding unit tests.