NotificationsYou must be signed in to change notification settings
Fork219
Star781

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

eddelbuettel merged 2 commits intoRcppCore:masterfromnathan-russell:master

Sep 5, 2016

Merged

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

eddelbuettel merged 2 commits intoRcppCore:masterfromnathan-russell:master

Sep 5, 2016

Conversation

Copy link

Contributor

nathan-russell commentedSep 4, 2016

As discussed in#549, this commit adds Sugar functions forrowSums,colSums,rowMeans, andcolMeans, along with unit tests.

row/col-Sums/Means and unit tests -RcppCore#549

e92b696

Copy link

Member

eddelbuettel commentedSep 4, 2016

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

kevinushey reviewed

Sep 5, 2016

View reviewed changes

inst/include/Rcpp/sugar/functions/rowSums.h Outdated

		}


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

Copy link

Contributor

kevinusheySep 5, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 thatdo_something(x) will not mutatex, whereby it's more obvious that mutation might happen withdo_something(&x).

The main downside is the fact that pointers can beNULL 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.

Copy link

Member

jjallaireSep 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 Usheynotifications@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
.