RFR Re: [PATCH] 8178117: Add public state constructors for Int/Long/DoubleSummaryStatistics

[joe darcy]

Hi Paul,


On 10/30/2017 9:33 PM, Paul Sandoz wrote:
> Hi,
>
>> On 30 Oct 2017, at 17:19, Joseph D. Darcy <joe.darcy at oracle.com> wrote:
>>
>> Hello,
>>
>> Is it intentional that "DoubleSummaryStatistics" is used in a sentence like
>>
>>    91      * @apiNote
>>    92      * The enforcement of argument correctness means that the retrieved set of
>>    93      * recorded values obtained from a {@code DoubleSummaryStatistics} source
>>
>> in all three types being updated?
>>
> No, copy’n’paste error. Fixed.
>
>
>> For the code in the constructor, if you don't want to have something like an explicit switch on Long.signum(count), I'd prefer to see at least a comment like
>>
>> // Use default field values if count == 0
>>
> Done.
>
>
>> For the double case, I'd recommend future work change the new constructor to
>>
>>      DoubleSummaryStatistics(long count, double min, double max, double sum, double... additionalSum)
>>
>> where the final argument could be used to convey additional state.
>>
> Ok.
>
>
>> For the double case, if a NaN is used than max and min will be NaN. Therefore, I'd recommend change the correctness constraint for that type to
>>
>>      <li>{@code min} ≤ {@code max} || (isNaN(min) && isNaN(max)) </li>
>>
>> with an analogous update to the code.
>>
> In that case we need to double (sorry) down on the NaNs and include sum as well:
>
> *   <li>{@code (min <= max && !isNaN(sum)) || (isNaN(min) && isNaN(max) && isNaN(sum))}

A more complete test for the numerical consistency conditions would be 
something like

     min <= sum/count  <= max

However, that would require a bit of thought due to potential round-off 
so this might not be worth the complexity trade-off. (If any of min, 
sum, and max were NaN, the comparisons would be false.)

Cheers,

-Joe