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

On 11/2/2017 9:58 AM, Paul Sandoz wrote: On 31 Oct 2017, at 11:24, Brian Goetz wrote: While I agree that the overflow-detecting constraint on min/max in the early version was too aggressive, you could reasonably choose to enforce the constraint that if count == 0, then so should sum, min, and

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

> On 31 Oct 2017, at 11:24, Brian Goetz wrote: > > While I agree that the overflow-detecting constraint on min/max in the early > version was too aggressive, you could reasonably choose to enforce the > constraint that if count == 0, then so should sum, min, and max. > I’m on the fence on th

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

Just to confirm this looks fine to me. From my point of view too much input validation would seem a little odd given that the implementation does nothing to protect itself from overflow in the first place. > On Nov 1, 2017, at 1:21 PM, Paul Sandoz wrote: > > >> On 31 Oct 2017, at 16:46, joe

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

> On 31 Oct 2017, at 16:46, joe darcy wrote: >>> >> In that case we need to double (sorry) down on the NaNs and include sum as >> well: >> >> * {@code (min <= max && !isNaN(sum)) || (isNaN(min) && isNaN(max) && >> isNaN(sum))} > > A more complete test for the numerical consistency conditio

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

Hi Paul, On 10/30/2017 9:33 PM, Paul Sandoz wrote: Hi, On 30 Oct 2017, at 17:19, Joseph D. Darcy 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 s

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

While I agree that the overflow-detecting constraint on min/max in the early version was too aggressive, you could reasonably choose to enforce the constraint that if count == 0, then so should sum, min, and max. On 10/30/2017 6:49 PM, Paul Sandoz wrote: Hi Chris, After some hiatus here is an

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

Hi, > On 30 Oct 2017, at 17:19, Joseph D. Darcy 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 obta

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

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 be

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

Looks good but I was wondering why there are static imports of a couple of j.l.Math methods in Int/LongSummaryStatistics. Brian On Oct 30, 2017, at 4:23 PM, Paul Sandoz wrote: > Thanks! updated the webrev and CSR.

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

> On 30 Oct 2017, at 16:14, Brian Burkhalter > wrote: > > Hi Paul, > > On the specification, e.g., at line 95 of DoubleSummaryStatistics: > > s/recorded the count of values/recorded count of values/ > Thanks! updated the webrev and CSR. Paul. > Brian > > On Oct 30, 2017, at 3:49 PM, Paul

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

Hi Paul, On the specification, e.g., at line 95 of DoubleSummaryStatistics: s/recorded the count of values/recorded count of values/ Brian On Oct 30, 2017, at 3:49 PM, Paul Sandoz wrote: > And the CSR: > > > https://bugs.openjdk.java.net/browse/JDK-8190381

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

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.open

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

Hi Joe, On 04/12/2017 07:59 PM, Joseph D. Darcy wrote: On 4/11/2017 1:48 PM, Chris Dennis 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 limit

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

On 4/11/2017 1:48 PM, Chris Dennis 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

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

Something like that was what I was envisioning (of course the inability to represent NaN or infinity with a BigDecimal is annoying). To me this approach really only breaks down when you stop using an actual iteration to perform the computation… as long as a “sum” in some form is part of the inte

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

On 04/12/2017 04:41 PM, Peter Levart wrote: On 04/11/2017 10:48 PM, Chris Dennis 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 p

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

Hi Dennis, On 04/11/2017 10:48 PM, Chris Dennis 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 DoubleSummar

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

Updated patch with the changed parameter validation, updated javadoc and added test coverage. # HG changeset patch # User chris_dennis # Date 1491945909 14400 # Tue Apr 11 17:25:09 2017 -0400 # Node ID c87cb7f14db71cff2bb4f0a7490b77cfe7ac07b6 # Parent fbedc2de689fd9fe693115630225e2008827c4e

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

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 w

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

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

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

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 call

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

# HG changeset patch # User chris_dennis # Date 1491485015 14400 # Thu Apr 06 09:23:35 2017 -0400 # Node ID d789970b8393032457885e739d76919f714bbd50 # Parent c0aecf84349c97f4241eab01f7bbfb7660d51be1 8178117: Add public state constructors for Int/Long/DoubleSummaryStatistics diff --git a/src/

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

Hi Chris, Unfortunately the patch you sent (in what I presume was an attachment) is missing. I believe the OpenJDK mailing list servers intentionally strip out attachments in all emails, which seems to be at odds with the advice given in http://openjdk.java.net/contribute/. (Either the contributio

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

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