| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
See my early comment on #95861 I think we should make _rank public, and remove the "by_rank" parameter to correlation, moving that functionality into a thin wrapper function spearman. Otherwise this is good work, thank you. |
Sorry, something went wrong.
There was a problem hiding this comment.
If you categorically disagree with my suggestion to make rank public and move spearman into its own named function, I will accept this (you've done the hard work writing the code and I do like the feature). But I do strongly request that we keep to the design of separate named functions rather than overloading the one function with a flag.
If you categorically want to keep the single correlation function, then we could future-proof it and make the type of correlation more explicit with an enumerated flag rather than a bool:
def correlation(x, y, /, kind='pearson'):
if kind == 'spearman':
x = _rank(x)
y = _rank(y)
elif kind != 'pearson':
raise ValueError('unsupported correlation kind')
If we ever add another sort of correlation (say, Kendall tau) is would be awkward to handle a second bool flag.
Sorry, something went wrong.
|
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again. |
Sorry, something went wrong.
|
Thank you for responding quickly and thoughtfully. I added some comments to the issue to help us get to a good meeting of the minds on the API. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Still need to think about whether by_rank is the best name for the flag.