Hi,

> On 30 Oct 2017, at 17:19, Joseph D. Darcy <joe.da...@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} &le; {@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))}</li>

I updated the webrev and added some more tests:

  
http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8178117-summary-constructors/webrev/

Thanks,
Paul.



> Thanks,
> 
> -Joe
> 
> On 10/30/2017 3:49 PM, Paul Sandoz wrote:
>> Hi Chris,
>> 
>> After some hiatus here is an updated webrev, i made some tweaks to the 
>> JavaDoc:
>> 
>>   
>> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8178117-summary-constructors/webrev/
>> 
>> And the CSR:
>> 
>>   https://bugs.openjdk.java.net/browse/JDK-8190381
>> 
>> The support for a double sum of consisting of an array of double is going 
>> beyond my complexity budget for this feature. If that is deemed important 
>> later on we can add another constructor.
>> 
>> Paul.
>> 
>>> On 11 Apr 2017, at 17:44, Chris Dennis <chris.w.den...@gmail.com> wrote:
>>> 
>>> Updated patch with the changed parameter validation, updated javadoc and 
>>> added test coverage.
>>> 
>>> <8178117.2.patch.txt>
>>> 
>>>> On Apr 11, 2017, at 4:48 PM, Chris Dennis <chris.w.den...@gmail.com> wrote:
>>>> 
>>>> Color me confused… what would the javadoc on the parameter say? It could I 
>>>> guess have an @implNote documenting the meanings of the parameters… but 
>>>> then what use is it? The proposed API simply limits the precision with 
>>>> which a DoubleSummaryStatistic can be copied to be the same as the 
>>>> precision with which it can be “accessed”.  That doesn’t seem an enormous 
>>>> problem since I suspect that bulk of usages would be to marshall a 
>>>> “finished” instance and therefore there is no real loss occuring. If we 
>>>> wanted to support arbitrary precision wouldn’t it be better to have a 
>>>> constructor variant that took a BigDecimal, and a matching getPreciseSum() 
>>>> that returned a BigDecimal?
>>>> 
>>>> Chris
>>>> 
>>>>> On Apr 11, 2017, at 4:16 PM, joe darcy <joe.da...@oracle.com> wrote:
>>>>> 
>>>>> On an API design note, I would prefer to DoubleSummaryStatistics took a 
>>>>> double... argument to pass in the state of the summation. This 
>>>>> flexibility is necessary to more fully preserve the computed sum. Also, 
>>>>> the we want flexibility to change the amount of internal state 
>>>>> DoubleSummaryStats keeps so we don't want to hard-code that into as 
>>>>> aspect of the API.
>>>>> 
>>>>> Thanks,
>>>>> 
>>>>> -Joe
>>>>> 
>>>>> 
>>>>> On 4/11/2017 12:53 PM, Paul Sandoz wrote:
>>>>>> Hi Chris,
>>>>>> 
>>>>>> Thanks for looking at this.
>>>>>> 
>>>>>> There is some rudimentary testing using streams in 
>>>>>> CollectAndSummaryStatisticsTest.java.
>>>>>> 
>>>>>> I think we should enforce constraints where we reliably can:
>>>>>> 
>>>>>> 1) count >= 0
>>>>>> 
>>>>>> 2) count = 0, then min/max/sum are ignored and it’s as if the default 
>>>>>> constructor was called. (I thought it might be appropriate to reject 
>>>>>> since a caller might be passing in incorrect information in error, but 
>>>>>> the defaults for min/max are important and would be a burden for the 
>>>>>> caller to pass those values.) In this respect having count as the first 
>>>>>> parameter of the constructor would be useful.
>>>>>> 
>>>>>> 3)  min <= max
>>>>>> 
>>>>>> Since for count > 0 the constraints, count * max >= sum, count * min <= 
>>>>>> sum, cannot be reliably enforced due to overflow i am inclined to not 
>>>>>> bother and just document.
>>>>>> 
>>>>>> 
>>>>>> Note this is gonna be blocked from pushing until the new Compatibility 
>>>>>> and Specification Review (CSR) process is opened up, which i understand 
>>>>>> is “soon"ish :-) but that should not block adding some further tests in 
>>>>>> the interim and tidying up the javadoc.
>>>>>> 
>>>>>> Paul.
>>>>>> 
>>>>>> 
>>>>>>> On 6 Apr 2017, at 07:08, Chris Dennis <chris.w.den...@gmail.com> wrote:
>>>>>>> 
>>>>>>> Hi Paul (et al)
>>>>>>> 
>>>>>>> Like all things API there are wrinkles here when it comes to 
>>>>>>> implementing.
>>>>>>> 
>>>>>>> This patch isn’t final, there appears to be no existing test coverage 
>>>>>>> for these classes beyond testing the compensating summation used in the 
>>>>>>> double implementation, and I left off adding any until it was decided 
>>>>>>> how much parameter validation we want (since that’s the only testing 
>>>>>>> that can really occur here).
>>>>>>> 
>>>>>>> The wrinkles relate to the issues around instances that have suffered 
>>>>>>> overflow in count and/or sum which the existing implementation doesn’t 
>>>>>>> defend against:
>>>>>>> 
>>>>>>> * I chose to ignore all parameters if 'count == 0’ and just leave the 
>>>>>>> instance empty. I held off from validating min, max and count however. 
>>>>>>> This obviously 'breaks things’ (beyond how broken they would already 
>>>>>>> be) if count == 0 only due to overflow.
>>>>>>> * I chose to fail if count is negative.
>>>>>>> * I chose to enforce that max and min are logically consistent with 
>>>>>>> count and sum, this can break the moment either one of the overflows as 
>>>>>>> well (I’m also pretty sure that the FPU logic is going to suffer here 
>>>>>>> too - there might be some magic I can do with a pessimistic bit of 
>>>>>>> rounding on the FPU multiplication result).
>>>>>>> 
>>>>>>> I intentionally went with the most aggressive parameter validation here 
>>>>>>> to establish one end of what could be implemented.  There are arguments 
>>>>>>> for this and also arguments for the other extreme (no validation at 
>>>>>>> all).  Personally I’m in favor of the current version where the 
>>>>>>> creation will fail if the inputs are only possible through overflow 
>>>>>>> (modulo fixing the FPU precision issues above) even though it’s at odds 
>>>>>>> with approach of the existing implementation.
>>>>>>> 
>>>>>>> Would appreciate thoughts/feedback.  Thanks.
>>>>>>> 
>>>>>>> Chris
>>>>>>> 
>>>>>>> 
>>>>>>> P.S. I presume tests would be required for the parameter checking (once 
>>>>>>> it is finalized)?
>>>>>>> 
>>>>>>> <8178117.patch>
>>>>> 
>>>> 
>>> 
>> 
> 

Reply via email to