rolling mean

[jangorecki]

Address #2778
Proposed frollmean implementation, all feature to be supported are implemented, also docs. When it will pass review I will re-use it for rest of rolling functions, and then prepare for 1.12.0.

[arunsrinivasan]

Very nice! Will take a look at this ASAP and write back.

[jangorecki]

just pushed update

• rebased to recent master (previous inline comments are gone, but all were addressed)
• uses memmove instead of memcpy
• fixed compiler warning
• proper way of nested parallelism
• now parallel will work with verbose=T when there is no print inside parallel region
• reorg code for less repetition, C level wrappers
• some segfault fixes

code for benchmarking landed in https://gist.github.com/jangorecki/96d4956dbf9cf0de118655ecbaca9b73
may require change from exact=TRUE to algo="exact"

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@c434610). Click here to learn what that means.
The diff coverage is 95.85%.

@@            Coverage Diff            @@
##             master    #2961   +/-   ##
=========================================
  Coverage          ?   93.69%           
=========================================
  Files             ?       65           
  Lines             ?    11949           
  Branches          ?        0           
=========================================
  Hits              ?    11196           
  Misses            ?      753           
  Partials          ?        0

Impacted Files	Coverage Δ
src/init.c	93.65% <ø> (ø)
R/froll.R	100% <100%> (ø)
src/frollR.c	100% <100%> (ø)
src/frolladaptive.c	88.18% <88.18%> (ø)
src/froll.c	98.14% <98.14%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c434610...31dca17. Read the comment docs.

[mattdowle]

Starting to look through this. Just quick initial comments :
Can't see a NEWS item.
Suggest adding "experimental function names and arguments; new in v1.12.0" to the top of the manual pages so that it's clear that it's new and might change.
I saw two malloc calls that don't have return value checked. Those need if (==NULL) error("alloc failed...) adding, or change to R_alloc() is preferable where possible.

[jangorecki]

@mattdowle Thanks for looking into this. NEWS entry added, also checks for malloc. I did not use R_alloc because those functions are attempting to be R agnostic.

As of now, it might happened that error("could not allocate...") will be raise from parallel region. The one defined in frollfunR when user provides 2+ columns to roll on AND/OR 2+ window sizes for rolling. There is nested parallel region in fadaptiverollmeanFast function where allocation happens but allocation is made before parallel region is entered there. Still it might be in parallel region of parent function frollfunR. Do I understand correctly this is not safe? If so I had to either

• move those allocations to frollfunR before entering topmost parallel region. One of the allocation occurs only if NAs were detected so might ended up unnecessarily allocating extra space that won't be used.
• implement error handling from parallel region by returning some kind of ans_t struct storing status, error message, and results (when no exception occurred).

I think the latter one is way to go, but it will be definitely more complex change comparing to just moving allocation. Looking forward for your feedback.

Also, before merging we may want to move tests from own test file to tests.Rraw.

[mattdowle]

Yes, the malloc itself is thread-safe (thread lock inside it). It's just the R API error() that isn't. Allocating up front before parallel region is usually best. Even a large malloc() will be almost instant so there's no time lost if it's not used; just higher memory usage and the possibility of failing to allocate something that isn't needed. Only at the point of touching allocated memory does anything happen (e.g. calloc vs malloc).

[jangorecki]

I went for the second option. First option would be easier but it would be polluting code as those two mallocs were specific to the algorithm (as of now only 1 out of 4 rolling statistics algos in this PR). Same frollfunR is going to be used for other rolling statistics so polluting it with pointers needed for specific algos in specific statistics won't scale.

Added exception handling so fails of malloc are handled using this exception handling mechanism. Additionally one type of console output (before printed with verbose) is now warning. It should be warning but I simplified it to not need to handle exceptions. Unit test for that verbose output had comment # this could be warning but no warns from plain C code. Because now we have exception handling mechanism I turned those console outputs into warnings. There is a limit on how long error/warning string can be, set to 255, which is safe because that string does not depend on user input.
Build and install using gcc-7 going perfectly well. When using cc(F) with same gcc I am getting following warning: SOLVED

Also checked that wrapping double vector for answer into struct does not degrade performance.

[jangorecki]

@mattdowle Currently this field is only used for storing errors and warnings, which won't exceed 255 chars. For storing console output for verbose=T (so verbose can work from parallel region) we will need to extend size of that char. Ideally we need to make it dynamic. For this PR this is enough as verbose=T uses Rprintf in real-time but disables topmost parallelism, and nested parallelism (where applicable) is not using verbose from parallel region. The cost of that change will be that verbose will not print in real-time, as opposed to fread which uses sophisticated method for printing safely and still do parallel processing. Here it is not necessary because it runs very fast.

[mattdowle]

Looks good.
Since there are so many new tests, makes sense to keep them in separate file like that, good.
I did ask for the manual page to state "experimental" at the top but I only see that stated in the news item. As long as that's added before release please. Also should they type ?froll or ?frollmean could be stated in the NEWS item so they know exactly what to type to find out about it and see the examples.