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?
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
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.
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.
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/
<http://cr.openjdk.java.net/%7Epsandoz/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
<mailto: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
<mailto: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
<mailto: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
<mailto: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>
