Remove 'sample' parameter from stats::mean API #2389
Conversation
… KahanBabushkaNeumaierSum to strided summation
… into kahan_neumeier_sum
mfoerste4
added
breaking
| @@ -27,10 +27,9 @@ namespace stats { | |||
| namespace detail { | |||
|
|
|||
| template <typename Type, typename IdxType = int> | |||
| void mean( | |||
| Type* mu, const Type* data, IdxType D, IdxType N, bool sample, bool rowMajor, cudaStream_t stream) | |||
| void mean(Type* mu, const Type* data, IdxType D, IdxType N, bool rowMajor, cudaStream_t stream) | |||
There was a problem hiding this comment.
What's the reason for removing the ability to compute a sample-based mean vs a population-based mean? This feature seems useful.
There was a problem hiding this comment.
This is correct for some statistical metrics like stddev or var, but IIUC that theory does not hold for the mean. The sample mean is the closest we have to the population mean.
There was a problem hiding this comment.
Both sample and population mean shall have 1/N factor. That gives the unbiased estimate. (This is unlike stdev or variance where we need to use different factor for sample and population mean).
There was a problem hiding this comment.
Thanks Malte for this PR. I agree that using 1/(N-1) factor for sample mean is not correct, we should have a single option that has 1/N factor. Therefore it makes sense to remove the sample parameter.
One should still consider that this is a breaking change, so we can consider to postpone this PR for 24.10, and issue a warning in 24.08 if the sample param is used.
This was the primary reason I pointed this out. We need to be careful about updates to public apis specifically, because we can only assume they are being used downstream (and we need to check). |
This PR removes the sample-parameter from the
raft::stats::meanAPI to prevent people from using it by accident when for example computing the mean for a sampled variance computation.This also invalidates some of the testcases. Within raft only test-code is affected by this change as the active usage of the sample parameter was already removed in #2381.
This PR is based on #2381 but was separated for tracking purposes.
Note that this requires adaption of downstream libraries using the API. I am aware of at least one occurrence in
cuml.