ENH: Implement Kendall discordance - #110
Conversation
There was a problem hiding this comment.
This looks like a faithful translation, and the tests are OK with my if they're ok with @steppi.
The tests are a little light, so I would suggest a few larger, non-trivial examples (probably randomly generated), and also checking behavior when there are tied elements. Maybe run kendalltau with randomly generated x and y (with and without the possibilities of ties in x, ties in y, and both) up until you get to dis = _kendall_dis(x, y) # discordant pairs to see what sort of input _kendall_dis can receive.
Thank you!
@steppi if we use this in SciPy, can it automatically be vectorized to work along the last axis?
Thanks @mdhaber, I'll improve the tests. |
fbourgey
changed the title
|
@mdhaber I have improved the tests with randomly-generated and tie/no-tie scenarios for x and y. Can you detail exactly what you meant with
|
It's probably not very important. I just couldn't remember exactly what could happen with the input to |
@mdhaber OK, thanks. Do you think the current test is sufficient? Happy to expand on it if needed. |
|
@dschmitz89 when you get the chance, can you please review that one? |
|
|
||
| inline double gdtrc(double a, double b, double x) { return cephes::gdtrc(a, b, x); } | ||
|
|
||
| inline int64_t kendall_dis(const std::vector<intptr_t> &x, const std::vector<intptr_t> &y) { |
There was a problem hiding this comment.
Does this need to be CuPy compatible in future? then std::vector is a no go from what I understood.
There was a problem hiding this comment.
Right, and I have cupy/cupy#9959 working now, so it's something we can actually try out now off of that branch.
There was a problem hiding this comment.
This should take three mdspan views, for x and y, but also for the scratch buffer space arr. No allocations inside of kernels. The SciPy gufunc should use the stateful functor pattern from https://github.com/steppi/scipy/blob/759ab1c458d56404dc8938a6074ebec8d290b449/scipy/special/mathieu.h to reuse the scratch buffer over the course of a single gufunc call. The CuPy gufunc will pre-allocate the scratch buffer space in Python wrapper.
|
This LGTM, only question left is the CuPy compatibility. |
| inline double gdtrc(double a, double b, double x) { return cephes::gdtrc(a, b, x); } | ||
|
|
||
| inline int64_t kendall_dis(const std::vector<intptr_t> &x, const std::vector<intptr_t> &y) { | ||
| intptr_t sup = 1 + *std::max_element(y.begin(), y.end()); |
There was a problem hiding this comment.
Wait, I missed that the size of the scratch buffer depends on the values of y. I think the way this is done needs to be rethought. I'll look into it.
There was a problem hiding this comment.
It seems possible to use a different algorithm (Knight's merge-sort based algorithm) that won't require a buffer with size depending on the values of y. I'll take this one from here.
4 participants
Reference issue
Toward #98
What does this implement/fix?
Implement Kendall discordance from
_kendall_dis