Revert Cythonized Kendall implementation and improve test case to prevent regressions

[zrait]

• closes BUG: Cythonized kendall correlation implementation is incorrect #43401
• tests added / passed
• Ensure all linting tests pass, see here for how to run them
• whatsnew entry

[jreback]

instead of reverting would be better to actually fix it
this is an edge case

[zrait]

I don't actually agree that this is an edge case. Kendall correlation is intended to be used with ordinal data and a textbook application would be something like computing correlation between a survey response with a limited domain 1-10 and some other variable--a case in which you're almost certain to have many duplicates.

[zrait]

If you look at the scipy Kendall code it is pretty clever and well vectorized and also uses a Cythonized Fenwick tree to compute the discordant pairs.

Looking at the original diff here and the performance improvements cited on it, I actually think a significant amount of the benefit is that ignoring ties allows you to take some major shortcuts. I'll note that I'm not an experienced Cython optimizer so if someone can make this handle ties while retaining the same margin of performance improvements I'd be very impressed and agree that'd be better. My intuition is that no matter what, a correct implementation will still have appreciably worse performance than the current incorrect one.

[lithomas1]

I'm fine with reverting this.

[simonjayhawkins]

I'm fine with reverting this.

This is in released pandas so would also need a release note if we do this.

[zrait]

I'm fine with reverting this.

This is in released pandas so would also need a release note if we do this.

I added a release note referencing this is as fixing a regression.

To add a bit more to my above remarks, I think that in general if a change made solely to improve performance introduces incorrect behavior, unless the fix is trivial, the correct response is to first revert it and then reopen the original goal of improving performance as a separate issue. Incorrect code is worse than slow code, and the baseline for a new attempt at improving performance should be the original implementation (in this case the scipy path that was used prior to the Cython implementation), not the wrong-but-faster code.

[jreback]

To add a bit more to my above remarks, I think that in general if a change made solely to improve performance introduces incorrect behavior, unless the fix is trivial, the correct response is to first revert it and then reopen the original goal of improving performance as a separate issue. Incorrect code is worse than slow code, and the baseline for a new attempt at improving performance should be the original implementation (in this case the scipy path that was used prior to the Cython implementation), not the wrong-but-faster code.

of course, pandas is a very large project and if something is not tested (which is the case here), regressions are certainly possible. that's why we need external testing. so thank you for that. all that said, happy to have an implementation that is performant and is correct.

[jreback]

this is failing pre-commit: https://github.com/pandas-dev/pandas/pull/43403/checks?check_run_id=3526049796

[zrait]

this is failing pre-commit: https://github.com/pandas-dev/pandas/pull/43403/checks?check_run_id=3526049796

Oops, should be fixed now--accidentally removed an extra newline in pandas/_libs/algos.pyx

[jreback]

thanks @zrait

[jreback]

@meeseeksdev backport 1.3.x

[lumberbot-app]

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

$ git checkout 1.3.x
$ git pull

2. Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 daaf2868e0d8841b2d4cdaa4ca766f41fe8dc6c6

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #43403: Revert Cythonized Kendall implementation and improve test case to prevent regressions'

4. Push to a named branch :

git push YOURFORK 1.3.x:auto-backport-of-pr-43403-on-1.3.x

5. Create a PR against branch 1.3.x, I would have named this PR:

"Backport PR #43403 on branch 1.3.x (Revert Cythonized Kendall implementation and improve test case to prevent regressions)"

And apply the correct labels and milestones.

Congratulation you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove Still Needs Manual Backport label once the PR gets merged.

If these instruction are inaccurate, feel free to suggest an improvement.

[jreback]

@zrait if you wouldn't mind following the instructions above for the backport.

[lumberbot-app]

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

$ git checkout 1.3.x
$ git pull

2. Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 daaf2868e0d8841b2d4cdaa4ca766f41fe8dc6c6

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #43403: Revert Cythonized Kendall implementation and improve test case to prevent regressions'

4. Push to a named branch :

git push YOURFORK 1.3.x:auto-backport-of-pr-43403-on-1.3.x

5. Create a PR against branch 1.3.x, I would have named this PR:

"Backport PR #43403 on branch 1.3.x (Revert Cythonized Kendall implementation and improve test case to prevent regressions)"

And apply the correct labels and milestones.

Congratulation you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove Still Needs Manual Backport label once the PR gets merged.

If these instruction are inaccurate, feel free to suggest an improvement.

[zrait]

@zrait if you wouldn't mind following the instructions above for the backport.

#43431