refactor(python): Rename pearson_corr & spearman_rank_corr #7014
Conversation
github-actions
bot
added
the
refactor
josemasar
changed the title
josemasar
changed the title
| """ | ||
| if isinstance(a, str): | ||
| a = col(a) | ||
| if isinstance(b, str): | ||
| b = col(b) | ||
| return pli.wrap_expr(pypearson_corr(a._pyexpr, b._pyexpr, ddof)) | ||
| if method == "pearson": | ||
| return pli.wrap_expr(pypearson_corr(a._pyexpr, b._pyexpr, ddof)) |
There was a problem hiding this comment.
Does this method not support propagate_nans? That is not documented in the docstring currently, although I guess the best course of action is to support that actually.
There was a problem hiding this comment.
Added clarification in the docstrings.
| @@ -7407,7 +7407,7 @@ def unnest(self, names: str | Sequence[str]) -> Self: | |||
| return self._from_pydf(self._df.unnest(names)) | |||
|
|
|||
| @typing.no_type_check | |||
| def pearson_corr(self, **kwargs: Any) -> Self: | |||
There was a problem hiding this comment.
As on expressions, we need to keep this name and raise a deprecation warning.
| b: str | pli.Expr, | ||
| method: CorrelationMethod = "pearson", | ||
| ddof: int = 1, | ||
| propagate_nans: bool = False, |
There was a problem hiding this comment.
method, ddof and propogate_nans should be keyword only imo, but I guess that is what @stinodego is working on in another PR?
There was a problem hiding this comment.
Well, this is a new function, so let's get it right straight away. Indeed those should be keyword only.
| pl.pearson_corr("a", "b").alias("pearson"), | ||
| pl.spearman_rank_corr("a", "b").alias("spearman"), | ||
| pl.corr("a", "b").alias("pearson"), | ||
| pl.corr("a", "b", "spearman").alias("spearman"), |
There was a problem hiding this comment.
We should call method as a keyword argument, i.e. corr("a", "b", method="spearman"). Same below.
There was a problem hiding this comment.
Now it´s a keyword argument.
|
Im in favor of this change, as the Spearman rank correlation, although useful, is much less used vs the Pearson one, so it makes sense to hide that behind a kwarg. |
|
@stinodego @zundertj I´ve got some trouble resolving the merge conflicts. Can you please help me on this? |
|
I don't think I can solve it for you, since you're on a fork. I'd recommend updating your fork and then do an interactive rebase on master, only selecting the commits you need, and then fix conflicts for those. |
|
@stinodego Thanks for the tip! The PR is now ready for the final review. |
|
Thanks for the PR and the reviews. 👍 |
|
@ritchie46 I don't think this was ready to be merged, the old methods weren't deprecated correctly. Didn't get around to properly reviewing this yet. I can make a follow-up PR to fix it. |
Issue:
#6802