-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Further refactoring of reduce/mapreduce functions #7061
Conversation
if done(itr, s) | ||
error("argument is empty") | ||
end | ||
done(itr, s) && error("argument is empty") | ||
(v, s) = next(itr, s) | ||
vmin = v | ||
vmax = v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are supposed to come after the first while
loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When itr
is empty, there's no way to compute extremes (not even able to get v
from the first element). In such cases, we have no other choices than raising an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only vmin = v
and vmax = v
need to move. Otherwise the function returns NaN if the first element of the array is NaN, because the loop below only touches v
and not vmin
/vmax
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Thanks. I just made a commit that fixes this.
This looks good to me. It's quite impressive that you've managed cut away a third of this file. It might be good to move some of your description of the structure of the file in the PR into the code itself. |
Unfortunately I can't take the time to review this now, but such consistency and brevity sound great! |
There's no changes to API and this has been pretty well tested. If there is no objection, I will merge it tomorrow. |
Further refactoring of reduce/mapreduce functions
This was broken by #7061. I have added a test to avoid future breakage.
Originally, reduce and mapreduce functions are not implemented in a coherent way. Similar algorithms are repeatedly implemented for different functions, resulting in inconsistent behaviors, subtle bugs, and higher maintenance cost.
In this PR, we refactor the codebase for all these functions. The primary goal is to organize them into a coherent framework, without compromising flexibility and performance.
The core of this framework is the
mapreduce
function, defined as below:The
mapreduce
function dispatches the actual operation depending on the length of the input array:n = 0
: callmr_empty
to handle empty arrayn = 1
: callr_promote
to promote the value, in order to attain type stability2 <= n < 16
: use a simple loop to compute the result (since the array is small, it is not worth the efforts to call more sophisticated functions)n >= 16
: callmapreduce_impl
For different types of map-function
f
and reduce-operationop
, one can specializemr_empty
to handle empty arrays differently (it raises an error by default), and specializemapreduce_impl
to implement faster or more accurate algorithms for certain operations.For example, when
op
is of typeAddFun
,mr_empty
returns zero whenever appropriate, andmapreduce_impl
uses pairwise summation with four-way unrolling. In another example, whenop
isAndFun
orOrFun
, themapreduce_impl
uses an algorithm that may terminate earlier without scanning the whole array.Other functions are merely one-liner that delegate to
mapreduce
:In this way, consistent behavior is achieved. For example, the following statements actually invoke the same underlying algorithm:
With this refactoring, the source file
reduce.jl
is cut from 696 lines to 444 lines (a net loss of over 250 lines).The test script
test/reduce.jl
was also re-organized and expanded, which provides better coverage of these functions.The refactoring pass all tests (including Steven's type stability tests).