row/col-Sums/Means and unit tests - for #549 #551

Merged
merged 2 commits into from Sep 5, 2016

Projects

None yet

4 participants

@nathan-russell
Contributor

As discussed in #549, this commit adds Sugar functions for rowSums, colSums, rowMeans, and colMeans, along with unit tests.

@eddelbuettel
Member

That looks like it is really nice work, once again. Thank so much!

@kevinushey kevinushey and 1 other commented on an outdated diff Sep 5, 2016
inst/include/Rcpp/sugar/functions/rowSums.h
+}
+
+inline bool check_na(Rboolean x) {
+ return x == NA_LOGICAL;
+}
+
+inline bool check_na(SEXP x) {
+ return x == NA_STRING;
+}
+
+inline bool check_na(Rcomplex x) {
+ return ISNAN(x.r) || ISNAN(x.i);
+}
+
+
+inline void incr(double& lhs, double rhs) {
@kevinushey
kevinushey Sep 5, 2016 edited Contributor

Minor style quip, but I think it's a preferable to receive arguments that will be mutated by pointer, rather than by reference -- it makes it more obvious at the call site whether a passed-in parameter might be mutated. For example,

do_something(x);

versus

do_something(&x);

In the first case, I think the expectation most would have is that do_something(x) will not mutate x, whereby it's more obvious that mutation might happen with do_something(&x).

The main downside is the fact that pointers can be NULL while references can't, but I think the clarity in code makes up for that. I'll let others weigh on whether that change is worth making.

@jjallaire
jjallaire Sep 5, 2016 Member

I agree with Kevin, especially in the case where by local idiom there is no
possibility of the pointer being NULL (which BTW would already be true if
you can pass a reference there).

On Sun, Sep 4, 2016 at 9:38 PM, Kevin Ushey notifications@github.com
wrote:

In inst/include/Rcpp/sugar/functions/rowSums.h
#551 (comment):

+}
+
+inline bool check_na(Rboolean x) {

  • return x == NA_LOGICAL;
    +}

+inline bool check_na(SEXP x) {

  • return x == NA_STRING;
    +}

+inline bool check_na(Rcomplex x) {

  • return ISNAN(x.r) || ISNAN(x.i);
    +}

+inline void incr(double& lhs, double rhs) {

Minor style issue, but I think it's a preferable to receive arguments that
will be mutated by pointer, rather than by reference -- it makes it more
obvious at the call site whether a passed-in parameter might be mutated.
For example,

do_something(x);

versus

do_something(&x);

In the first case, I think the expectation most would have is that
do_something(x) will not mutate x, whereby it's more obvious that
mutation might happen with do_something(&x).

The main downside is the fact that pointers can be NULL while references
can't, but I think the clarity in code makes up for that. I'll let others
weigh on whether that change is worth making.

โ€”
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/RcppCore/Rcpp/pull/551/files/e92b69626da0e6f08f105e01e2f1ff63988da193#r77463324,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGXx1FKiNKUquAVcJPoK1VR4pXRlPxMks5qm3KrgaJpZM4J0m43
.

@kevinushey kevinushey commented on the diff Sep 5, 2016
inst/include/Rcpp/sugar/functions/rowSums.h
+ \
+ for (j = 0; j < nc; j++) { \
+ for (i = 0; i < nr; i++) { \
+ current = ref(i, j); \
+ if (!detail::check_na(current)) { \
+ detail::incr(res[i], current); \
+ } \
+ } \
+ } \
+ \
+ return res; \
+ } \
+};
+
+ROW_SUMS_IMPL_RMNA(LGLSXP)
+ROW_SUMS_IMPL_RMNA(INTSXP)
@kevinushey
kevinushey Sep 5, 2016 Contributor

Let's #undef the macro after we're done with it.

@eddelbuettel
Member

No strong feelings on pointers (clearer as @kevinushey says) versus references (bettern 'modern' C++ ?).

@kevinushey kevinushey commented on the diff Sep 5, 2016
inst/include/Rcpp/sugar/functions/rowSums.h
+ detail::incr(res[i], ref(i, j)); \
+ } \
+ } \
+ \
+ for (i = 0; i < nr; i++) { \
+ if (na_flags[i].x) { \
+ res[i] = NA_INTEGER; \
+ } \
+ } \
+ \
+ return res; \
+ } \
+};
+
+ROW_SUMS_IMPL_KEEPNA(LGLSXP)
+ROW_SUMS_IMPL_KEEPNA(INTSXP)
@kevinushey
kevinushey Sep 5, 2016 Contributor

Same #undef here?

@kevinushey kevinushey commented on an outdated diff Sep 5, 2016
inst/include/Rcpp/sugar/functions/rowSums.h
+colMeans(const MatrixBase<RTYPE, NA, T>& x, bool na_rm = false) {
+ if (!na_rm) {
+ return sugar::ColMeansImpl<RTYPE, NA, T, false>(x);
+ }
+ return sugar::ColMeansImpl<RTYPE, NA, T, true>(x);
+}
+
+
+#undef ROW_SUMS_IMPL_KEEPNA
+#undef ROW_SUMS_IMPL_RMNA
+#undef COL_SUMS_IMPL_KEEPNA
+#undef COL_SUMS_IMPL_RMNA
+#undef ROW_MEANS_IMPL_KEEPNA
+#undef ROW_MEANS_IMPL_RMNA
+#undef COL_MEANS_IMPL_KEEPNA
+#undef COL_MEANS_IMPL_RMNA
@kevinushey
kevinushey Sep 5, 2016 Contributor

Oh, I see now that everything is #undefed at the bottom here. Marginal preference to #undef immediately after you know you don't need it anymore, but I think this is fine too.

@kevinushey
Contributor

Barring some very minor stylistic suggestions, LGTM!

@eddelbuettel
Member

Thanks @kevinushey . We'll see if @nathan-russell has time for a revision, else we can probably take it as is.

@nathan-russell
Contributor

Those are reasonable suggestions; thanks for reviewing everyone.

@eddelbuettel eddelbuettel merged commit f75ff3b into RcppCore:master Sep 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment