Fix percentile and quantile #46
Conversation
| q = np.true_divide(q, 100) | ||
| # Use dtype of array if possible (e.g., if q is a python int or float) | ||
| # by making the divisor have the dtype of the data array. | ||
| q = np.true_divide(q, a.dtype.type(100) if a.dtype.kind == "f" else 100) |
There was a problem hiding this comment.
Hmmm, feels a bit brittle, but I have to sleep on it probably (also doesn't generalize easily to user DTypes, but OK).
Maybe in this case it is actually easier to just track was_pyscalar = type(q) in (int, float, complex) and then apply if was_pyscalar: pos = float(pos) at the end of the calculation.
Beyond figuring this out, one poitn I am annoying about it is, that I think this is an important problem to get eyes on, because percentile can't be the only function in NumPy (and even more so downstream!) to run into this.
There was a problem hiding this comment.
The problem with was_pyscalar is that I think the expectation should be that the result keeps the array input dtype, but only if it is float.
There was a problem hiding this comment.
Ah, by pos there, I don't mean the end-result, but the interpolation point value that is used for the last bit of the calculation. I.e. the intermediate result just before mixing it with the values from a.
There was a problem hiding this comment.
Ah, I see. Indeed, that could work too. I think I slightly prefer to trying to deal with things up front - I'm trying to think of these functions as gufuncs and following the same logic...
| q = np.asanyarray(q) | ||
| # Use dtype of array if possible (e.g., if q is a python int or float). | ||
| if isinstance(q, (int, float)) and a.dtype.kind == "f": | ||
| q = np.asanyarray(q, dtype=np.result_type(a, q)) |
There was a problem hiding this comment.
We could also just use this stanza for percentile to keep things more uniform.
Note that this would not work for user types either given the check for a.dtype.kind == "f", but I did have to include that since otherwise an example with a datetime broke.
|
Going to merge and roll with this for now. I am not sure I like it, but I suspect it is good enough either way. |
Discussion in main thread