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>
