On Dec 1 2013, at 23:34 , Joe Darcy <joe.da...@oracle.com> wrote: > Hello, > > I've heard back from my numerical reviewer. Given the signs of the terms, he > recommend when two partial sums are combined, the leading word be added in > first. I've reflected this change in the revision I'm about to push: > > http://cr.openjdk.java.net/~darcy/8006572.5 > > He had some suggestions for further numerical improvements, which I've > captured in > > JDK-8029372 Improve combination of partial sums/averages in streams API > https://bugs.openjdk.java.net/browse/JDK-8029372
Any description of what the improvements would be? The bug doesn't currently capture what will be changed/improved. > > Thanks, > > -Joe > > On 11/25/2013 09:57 AM, Mike Duigou wrote: >> Looks good >> >> Mike >> >> On Nov 22 2013, at 23:06 , Joe Darcy <joe.da...@oracle.com> wrote: >> >>> Hello Mike and Paul, >>> >>> Thanks for reviewing the earlier versions of the change; I've posted an >>> update revision for your consideration at >>> >>> http://cr.openjdk.java.net/~darcy/8006572.4/ >>> >>> Two numerical aspects of the change are different in this iteration: >>> >>> * when two running compensated sums and combined, the compensation >>> portion of the second is now added in first. Since it should have smaller >>> magnitude, this is favorable in terms of error analysis >>> >>> * the final sum returned (or used to compute the average) is (sum + >>> sumCompensation). I checked one of my references, "Accuracy and Stability >>> of Numerical Algorithms" from Nicholas Higham, and that tweak gave a better >>> error bound >>> >>> The test code has been updated as you've suggested (other than porting to >>> TestNG ;-) and I also added a test case for a bug Paul spotted on an >>> earlier review. >>> >>> All the streams tests pass with the new code. >>> >>> Thanks, >>> >>> -Joe >>> >>> On 11/20/2013 11:37 AM, Joe Darcy wrote: >>>> Hi Mike, >>>> >>>> On 11/20/2013 10:31 AM, Mike Duigou wrote: >>>>> - Collectors averagingDouble could use the same @implNote as in >>>>> DoublePipeline. >>>>> >>>>> - DoublePipeline implementation could document the usage of the double >>>>> array indices. >>>>> >>>>> - @summary in test. >>>> I'll reflect these in the next iteration. >>>> >>>>> - I agree with Paul that refactoring as a testNG test would be nice. >>>> While learning about testNG would be useful, I don't want to take that on >>>> right at the moment ;-) >>>> >>>>> - I wondered at the mechanism for combining compensation values. Do you >>>>> have a reference which explains the correctness? I thought perhaps >>>>> directly adding the compensations together before doing compensated >>>>> addition of the two sums might be better. >>>> One of the inquiries I have out to my numerical reviewer is how to best >>>> combine two compensations. >>>> >>>> In the code as written, from the perspective of one compensation, the two >>>> doubles in the other other compensation are just two more double values. >>>> So I think this is correct, if not ideal. >>>> >>>> The compensation portion of one running sum could be of vastly different >>>> magnitude than the compensation portion (or the high-order sum bits) of >>>> the other sum. Therefore, I believe a direct combination of either (sum1 + >>>> sum2) or (comp1 + comp2) would lose the numerical properties of interest >>>> here. >>>> >>>> Thanks for the feedback, >>>> >>>> -Joe >>>> >>>>> Mike >>>>> >>>>> >>>>> On Nov 20 2013, at 00:21 , Joe Darcy <joe.da...@oracle.com> wrote: >>>>> >>>>>> Hi Paul, >>>>>> >>>>>> On 11/18/2013 07:38 AM, Paul Sandoz wrote: >>>>>>> Hi Joe, >>>>>>> >>>>>>> You can use the three arg form of collect for >>>>>>> DoublePipeline.sum/average impls, which is already used for average: >>>>>>> >>>>>>> public final OptionalDouble average() { >>>>>>> double[] avg = collect(() -> new double[2], >>>>>>> (ll, i) -> { >>>>>>> ll[0]++; >>>>>>> ll[1] += i; >>>>>>> }, >>>>>>> (ll, rr) -> { >>>>>>> ll[0] += rr[0]; >>>>>>> ll[1] += rr[1]; >>>>>>> }); >>>>>>> return avg[0] > 0 >>>>>>> ? OptionalDouble.of(avg[1] / avg[0]) >>>>>>> : OptionalDouble.empty(); >>>>>>> } >>>>>>> >>>>>>> That would be the most expedient way. >>>>>> Thanks for the tip. For the moment, I'm feeling a bit expedient and used >>>>>> the array-based approach in an iteration of the change: >>>>>> >>>>>> http://cr.openjdk.java.net/~darcy/8006572.3/ >
