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} ≤ {@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> >>>>> >>>> >>> >> >