Hi,
> On 30 Oct 2017, at 17:19, Joseph D. Darcy <[email protected]> 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))}</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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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>
>>>>>
>>>>
>>>
>>
>
