nit: non-infinite-> finite probably

Probably sufficient to just check a single index rather than all of them. For the message check, I think we usually prefer using the match builtin to pytest.raises these days.

could parametrize the test to check one weights array with a few NaNs and another with just 1, but probably don't need these two exhaustive loops

Could also probably just parametrize over np.nan and np.inf + expected message above for concision (and same comment about not needing the exhaustive loops

Thanks, will do

A general comment: I think these checks should happen inside _weights_ar_valid - this will ensure they are used for percentile as well.

I'd parametrize over np.quantile and np.percentile as well - they should have the same errors.

Also, if you pass in a list rather than an array, you could parametrize over dtype=float and dtype=object, to make this a little more readable.

Done

Another general comment: as written, the common case has to go through a lot of checks. I think it would be better to optimize for the common case, and not worry too much about distinguishing failure cases. E.g., you can do just one evaluation with:

if not np.all(np.isfinite(weights)):
    raise ValueError("weights must be finite.")

Note that this loop can still give unexpected errors, because you are here counting on object arrays to be turned into their values as scalars. E.g.,

np.isnan(np.array([1.,None,np.inf])[1])
# TypeError: ufunc 'isnan' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''

This will be an uninformative error!

I think we have two choices: just not check for object dtype, or convert to float before checking (and then passing on it that conversion fails).

Here again we could ensure the common case remains fast by doing:

if np.any(weights <= 0):
    raise ValueError("weights must be non-negative and cannot be all zero.")
    # or, more explicit error messages,
    if np.all(weights == 0):
        raise ValueError("At least one weight must be non-zero.")
    else:
        raise ValueError("Weights must be non-negative.")

Trying to keep this inline:

The issue with this is that some of the weights might be 0, but none of them are negative. So it would raise an error even though it shouldn't

You're right, I was too sloppy in writing this, the else should be elif np.any(weights <0) so that the case of some weights 0 falls through (slowly, but better than making all cases slow!).

p.s. Given this, I'd probably swap the order, i.e.,

if np.any(weights <= 0):
    # Do these checks guarded by the above `if` to avoid slowing down the common case.
    if np.any(weights < 0):
        raise ValueError("Weights must be non-negative.")
    elif np.all(weights == 0):
        raise ValueError("At least one weight must be non-zero.")

@Tontonio3 I don't see how you responded to this suggestion. Please make sure all reviewer feedback is addressed before requesting re-review.

@ngoldbaum Your're right, I forgot to implement this

can't you just say else here?

I can't just say else here, as the first if will be true if only one of the weights is 0, which is valid

you can move the == 0 to the sum to simplify this, though.

IMO the checks may be clearer as not all(weights >= 0) to reject NaNs right away, but doesn't matter much so long the == 0 case is handled below.

Also, why not use isfinite?

If the weights are non-zero, but the array that it is given in all zeroes, the sum will be zero. I know it is a specific edge case, but it would give an error when it shouldn't give one.

I don't understand what the array matters, the below calculation is on weights?

The point is that after the cumsum, we need to check the cumsum result is finite and not zero. In fact you could even do that after normalization (at that point it is always NaN, although a warning would have occurred on the way if the input was inf or zero and not NaN).

Here, the check is incorrect. It checks that all weights are zero, but the thing we need to check is that if any slice is empty. Only the negative makes sense here.

might as well use match here too

I don't understand what the array matters, the below calculation is on weights?

The point is that after the cumsum, we need to check the cumsum result is finite and not zero. In fact you could even do that after normalization (at that point it is always NaN, although a warning would have occurred on the way if the input was inf or zero and not NaN).

Here, the check is incorrect. It checks that all weights are zero, but the thing we need to check is that if any slice is empty. Only the negative makes sense here.

File name is incorrect, should be 28595.improvement.rst.