Expose ApplyToCols and ApplyToFrame - #1478
Conversation
|
This will be a fun one to review |
|
ahahah indeed. I apologised in advance |
😅 thanks for plowing through |
| from ._join_utils import pick_column_names | ||
|
|
||
| __all__ = ["OnEachColumn", "SingleColumnTransformer", "RejectColumn"] | ||
| __all__ = ["ApplyToCols", "SingleColumnTransformer", "RejectColumn"] |
There was a problem hiding this comment.
total nitpick: maybe I would name that file _apply_to_cols.py?
|
Is this ready to review / merge? |
|
I'll review this tomorrow, I haven't had the time yet |
|
I'll review this tomorrow, I haven't had the time yet
Thanks Riccardo!!!
|
Co-authored-by: Riccardo Cappuzzo <7548232+rcap107@users.noreply.github.com>
| Xt | ||
|
|
||
| # %% | ||
| # Let's now imagine that our two previous columns ``"division"``, |
There was a problem hiding this comment.
I'm not convinced by this phrasing: either they are highly correlated (which they do not seem to be, from the associations) and so there is a reason for this operation, or they aren't. If they aren't, I think adding an imaginary reason would just add to confusion: it's an example, I think that showing what is happening is more useful
There was a problem hiding this comment.
Ok, I'll simplify the narrative
| # | ||
| # This time, we can use :class:`~skrub.ApplyToFrame`, which applies a transformer to | ||
| # columns jointly, rather than separately like :class:`~skrub.ApplyToCols`. | ||
| # |
There was a problem hiding this comment.
I think it would be useful to be more explicit, something like
This is useful for transformers that work on multiple columns at once, such as the PCA that we want to use to reduce the number of components.
| # We define a column filter using skrub selectors with a lambda function. | ||
| from sklearn.preprocessing import OrdinalEncoder | ||
|
|
||
| low_cardinality = s.filter(lambda col: col.nunique() < 40) |
There was a problem hiding this comment.
Given that selectors include .cardinality_below, I think there should either be a mention to the function ("this can also be done with .cardinality_below"), or a different example should be used
There was a problem hiding this comment.
True, I can mention it
There was a problem hiding this comment.
Should we address this?
(I also want terribly to merge, because this is very useful. If I merge too fast, maybe address in a new PR)
There was a problem hiding this comment.
yes it has been adressed in the text section above, L97:
# We define a column filter using skrub selectors with a lambda function. Note that
# the same effect can be obtained directly by using
# :func:`~srkub.selectors.cardinality_below`.
rcap107
left a comment
rcap107
left a comment
There was a problem hiding this comment.
Thanks a lot @Vincent-Maladiere, I went over all the files and they look good
Most of my comments are about the example, but overall it looks good 👍
rcap107
left a comment
rcap107
left a comment
There was a problem hiding this comment.
Looks great, thanks a lot @Vincent-Maladiere
GaelVaroquaux
left a comment
GaelVaroquaux
left a comment
There was a problem hiding this comment.
Merging!
Thanks a lot. This is a big deall
Co-authored-by: Riccardo Cappuzzo <7548232+rcap107@users.noreply.github.com>
Co-authored-by: Riccardo Cappuzzo <7548232+rcap107@users.noreply.github.com>
4 participants
Fixes #1429