New function psum/pprod #4188
New function psum/pprod #4188
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4188 +/- ##
=========================================
Coverage ? 99.61%
=========================================
Files ? 72
Lines ? 14028
Branches ? 0
=========================================
Hits ? 13974
Misses ? 54
Partials ? 0
Continue to review full report at Codecov.
|
src/fifelse.c
Outdated
| @@ -343,3 +343,161 @@ SEXP fcaseR(SEXP na, SEXP rho, SEXP args) { | |||
| UNPROTECT(nprotect); | |||
| return ans; | |||
| } | |||
|
|
|||
| SEXP psumR(SEXP na, SEXP args) { | |||
| if (!IS_TRUE_OR_FALSE(na)) { | |||
There was a problem hiding this comment.
We use 2 whitespace, you used 8. It is so much horizontal scrolling now.
There was a problem hiding this comment.
I will change all my c files. Thank you for letting me know.
|
Please note that we can had lines for openmp. Happy to do it if you want. These functions are already fast from an absolute point of view and are close to |
src/fifelse.c
Outdated
| } | ||
| SEXPTYPE anstype = TYPEOF(SEXPPTR_RO(args)[0]); | ||
| const int64_t len0 = xlength(SEXPPTR_RO(args)[0]); | ||
| if (anstype != INTSXP && anstype != REALSXP) { | ||
| error("Argument 1 is of type %s. Only integer and double types are supported.", type2char(anstype)); | ||
| error(_("Argument 1 is of type %s. Only integer and double types are supported."), type2char(anstype)); |
There was a problem hiding this comment.
for translation, better to use %d here as well, then only one message to translate instead of 2
There was a problem hiding this comment.
Sorry Michael, I did not understand what you mean.
There was a problem hiding this comment.
each unique item wrapped in _() will generate a message to be translated
one with Argument 1, another with Argument %d -- twice as much work for our translation team
vs using Argument %d for both, and hard-coding 1 to fill that in the former case; looks same to user, very similar in code, and easier for translators
src/fifelse.c
Outdated
| } | ||
| const int n=length(args); | ||
| if (n <= 1) { | ||
| error(_("Please supply at least 2 arguments. (%d argument supplied)"), n); |
There was a problem hiding this comment.
why error, instead of just returning x itself?
There was a problem hiding this comment.
I find it odd to provide 1 argument. A sum operation needs at least 2 arguments. Does it make sense to you to only provide 1 argument?
There was a problem hiding this comment.
yes, it's odd, but edge case like this can come up in dynamic scripting, where e.g. user is running
DT[ , do.call(psum, .SD), .SDcols = is.numeric]
and DT has a variable number of numeric columns depending on some other conditions in the script.
This way, the user can use the same code to cover the normal case when there are several normal columns, as well as the case when there only ends up being one such column.
There was a problem hiding this comment.
Ok will change that later. Tks
src/fifelse.c
Outdated
| "If you wish to 'recycle' your argument, please use rep() to make this intent " | ||
| "clear to the readers of your code."), i+1, len1, len0); | ||
| } | ||
| if (type > anstype) { |
There was a problem hiding this comment.
I think we should use typeorder for this, the ordering of the SEXPTYPEs doesn't necessarily reflect the coercion order. I guess it's fine for just int/double but later we may add other types
closes #3467. Towards #3189