Improved verbose messages in rolling functions frolladaptivefun and frollfun#7733
Improved verbose messages in rolling functions frolladaptivefun and frollfun#7733Omartech312 wants to merge 4 commits intoRdatatable:masterfrom
Conversation
Updating Project Fork
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7733 +/- ##
=======================================
Coverage 99.04% 99.04%
=======================================
Files 87 87
Lines 17058 17072 +14
=======================================
+ Hits 16895 16909 +14
Misses 163 163 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jangorecki
left a comment
jangorecki
left a comment
There was a problem hiding this comment.
Approach is good, just some comments to make it cleaner.
| "frollfun: processing fun MEAN algo fast took.*", | ||
| "frollfunR: processing.*took.*" | ||
| ), warning="has.nf=FALSE used but non-finite values are present in input, use default has.nf=NA to avoid this warning") | ||
| ), warning="has.nf=FALSE used but non-finite values are present in input, use default has.nf=NA to avoid this warning") #7021 |
| "frollfun: processing fun MEAN algo fast took.*", | ||
| "frollfunR: processing.*took.*" | ||
| )) | ||
| )) #7021 |
There was a problem hiding this comment.
I think we can safely skip issue markers in already existing tests
| if (verbose) | ||
| char rfunStr[7]; | ||
|
|
||
| const char *rfunNames[] = { |
There was a problem hiding this comment.
I think it make sense to define it once at compile time rather than inside frollrun function at runtime.
| char rfunStr[7]; | ||
|
|
||
| // enum ordered based from the file src/data.table.h | ||
| const char *rfunNames[] = { |
There was a problem hiding this comment.
Same here, if we will move it to data.table.h then we need to only declare it once
| if (verbose) | ||
| snprintf(end(ans->message[0]), 500, _("%s: processing fun %d algo %u took %.3fs\n"), __func__, rfun, algo, omp_get_wtime()-tic); | ||
| if (verbose) { | ||
| if(algo == 0) |
There was a problem hiding this comment.
I would go with ternary if operator, will be much smaller change
| //frolladaptivemaxFast(x, nx, ans, k, fill, narm, hasnf, verbose); // frolladaptivemaxFast does not exists as of now | ||
| snprintf(end(ans->message[0]), 500, _("%s: algo %u not implemented, fall back to %u\n"), __func__, algo, (unsigned int) 1); | ||
| snprintf(end(ans->message[0]), 500, _("%s: algo fast not implemented, fall back to exact\n"), __func__); | ||
| algo = 1; |
There was a problem hiding this comment.
Is this line needed/used? It wasn't there before, if it is not needed/used, then we could still have it, but then better commented out with a comment that it is not used.
| //frolladaptiveminFast(x, nx, ans, k, fill, narm, hasnf, verbose); // frolladaptiveminFast does not exists as of now | ||
| snprintf(end(ans->message[0]), 500, _("%s: algo %u not implemented, fall back to %u\n"), __func__, algo, (unsigned int) 1); | ||
| snprintf(end(ans->message[0]), 500, _("%s: algo fast not implemented, fall back to exact\n"), __func__); | ||
| algo = 1; |
| if (verbose) | ||
| snprintf(end(ans->message[0]), 500, _("%s: processing fun %d algo %u took %.3fs\n"), __func__, rfun, algo, omp_get_wtime()-tic); | ||
| if (verbose) { | ||
| if(algo == 0) |
| } | ||
| free(xx); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
This change should be reverted
|
|
||
| 5. `tables()` can now optionally report `data.table` objects stored one level deep inside list objects when `depth=1L`, [#2606](https://github.com/Rdatatable/data.table/issues/2606). Thanks @MichaelChirico for the report and @manmita for the PR | ||
|
|
||
| <<<<<<< HEAD |
There was a problem hiding this comment.
Move new entry below existing ones and clean code markers (<<<, ===, >>>) after git conflict.
4 participants
Closes #7021.
Previously, the frolladaptivefun and rfollfun algo verbose messages were not super user friendly, displaying messages like:
frolladaptivefun: algo 0 not implemented, fall back to 1The wanted adjustment was for it to instead produce:
frolladaptivefun: algo fast not implemented, fall back to exactThere was also:
frollfun: processing fun 0 algo 0 took...and the desired result was instead:
frollfun: processing fun mean algo fast took...We adjusted for the desired functionality by creating an array of strings called rfunNames, using the rfun enum as an index to select the proper display type for the running function and then displaying fast or exact dependent on the value of value (0 for fast, 1 for exact).
Here is an example of the current output (from running tests) with the changed files in this PR:
frolladaptivefun: processing fun MIN algo exact took 0.000sfrolladaptivefun: algo fast not implemented, fall back to exactWe have additionally adjusted the NEWS.md file and the corresponding tests in inst/tests/froll.Rraw.