8000 FEAT add the Frequency Encoder by coconutattitude · Pull Request #707 · skrub-data/skrub · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

FEAT add the Frequency Encoder #707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

FEAT add the Frequency Encoder #707

wants to merge 4 commits into from

Conversation

coconutattitude
Copy link

Addresses #706 issue.
Here's a first iteration of the class and it's a proof of concept made during EuroSciPy 2023. Documentation and tests are needed. I'll add quantile strategies in the future. What do you think about this proof of concept?

@Vincent-Maladiere
Copy link
Member

Thanks for this feature, @coconutattitude
Here is a quick roadmap to improve this PR before engaging in more formal reviews and discussions.

  1. Use pre-commit to linter and standardize your code (it is usually run during git commit).
    To do so:

    pip install -e '.[dev]'

    to install the dependencies used for development, then:

    pre-commit init

    to initialize pre-commit.

  2. Add a get_feature_name_out() method.
    This is useful for inspecting the data used to fit transformers.
    You can take inspiration from the GapEncoder method or DatetimeEncoder method

  3. Check for input types, i.e. what happens when X is a numpy array? What happens when it's a pandas dataframe?
    Most encoders/estimators across skrub use the check_input() utils function.

    skrub/skrub/_datetime_encoder.py

    Lines 192 to 196 in 9d2e64f

    if isinstance(X, pd.DataFrame):
    self.col_names_ = X.columns.to_list()
    else:
    self.col_names_ = None
    X = check_input(X)

  4. Add basic tests in a dedicated _test_frequency_encoder.py file test. This will help discover edge cases and situations in which the estimator would fail.

  5. Convert your example notebook into a Python file using jupytext

    pip install jupytext

    then:

    jupytext --set-formats ipynb,py:percent example/08_frequence_encoder.ipynb
jupytext --sync example/08_frequence_encoder.ipynb

    We prefer Python files over notebooks because we can review them in PR more easily, and they are more compact.

  6. Enrich your example by adding a regression/classification or even clustering task. Reproducing a similar use case to the one you mentioned IRL with IP addresses could bring value.

@Vincent-Maladiere Vincent-Maladiere changed the title add frequency encoder FEAT add the Frequency Encoder Aug 23, 2023
@Vincent-Maladiere
Copy link
Member

Hi @coconutattitude, are you planning to continue this PR?

@jeromedockes
Copy link
Member
jeromedockes commented Dec 3, 2024 •
edited
Loading

shall we try to revive this? :) it would be a nice addition

@Vincent-Maladiere
Copy link
Member

@rcap107 are you interested in continuing this PR once StringEncoder is merged?

@rcap107
Copy link
Contributor
rcap107 commented Dec 17, 2024

I can work on it, yes

@rcap107
Copy link
Contributor
rcap107 commented Mar 24, 2025

This feature was implemented in #1235

@rcap107 rcap107 closed this Mar 24, 2025
@Vincent-Maladiere
Copy link
Member

This feature was implemented in #1235

I don't think it was because this PR aims to encode categories using their frequencies; hence, I don't see how the periodic encoder solves this. WDYT?

@GaelVaroquaux
Copy link
Member
GaelVaroquaux commented Mar 24, 2025 via email

@rcap107
Copy link
Contributor
rcap107 commented Mar 26, 2025

This feature was implemented in #1235

I don't think it was because this PR aims to encode categories using their frequencies; hence, I don't see how the periodic encoder solves this. WDYT?

My bad, I mixed up this PR and the old discussion on the periodic encoder. This is absolutely unrelated from #1235

@rcap107
Copy link
Contributor
rcap107 commented Jun 11, 2025 •
edited
Loading

Hey, I'm picking up this one. Looking at the possible implementation, there are some points that we should consider:

  • Pandas accepts the number of bins, the bin edges or the exact bins to be used, while polars only accepts the bin edges.
  • What should we use as the interface for the user? The number of bins? The bin edges? If it's the number of bins, we need to add the computation of the edges.
  • We need to name the bins ourselves because the two libraries have different conventions
  • pl.cut is unstable according to the polars docs
  • Do we want to have both cut and qcut in the same transformer? I think it makes more sense like that

@rcap107 rcap107 self-assigned this Jun 11, 2025
@Vincent-Maladiere
Copy link
Member
Vincent-Maladiere commented Jun 11, 2025 •
edited
Loading

I think that identifying a dataset where this transformer performs well would strongly motivate the inclusion of this feature. It would also help us craft a narrative around it and clarify its role among the other encoders. If I recall correctly, OP was using it on IP addresses.

@Vincent-Maladiere
Copy link
Member
Vincent-Maladiere commented Jun 11, 2025 •
edited
Loading

What should we use as the interface for the user? The number of bins? The bin edges? If it's the number of bins, we need to add the computation of the edges.

I would go for the number of bins, which is easier to set and more intuitive than bin edges

Do we want to have both cut and qcut in the same transformer? I think it makes more sense like that

Do you mean cut for pandas and qcut polars?

@rcap107
Copy link
Contributor
rcap107 commented Jun 12, 2025 •
edited
Loading

I think that identifying a dataset where this transformer performs well would strongly motivate the inclusion of this feature. It would also help us craft a narrative around it and clarify its role among the other encoders. If I recall correctly, OP was using it on IP addresses.

Good point, I do not have good suggestions for that unfortunately 😅

What should we use as the interface for the user? The number of bins? The bin edges? If it's the number of bins, we need to add the computation of the edges.

I would go for the number of bins, which is easier to set and more intuitive than bin edges

I agree

Do we want to have both cut and qcut in the same transformer? I think it makes more sense like that

Do you mean cut for pandas and qcut polars?

No, I mean having both uniform and quantile bins in the same transformer, rather than having a transformer that does uniform binning and one that does quantile binning. There would be a flag that selects the transformer to use. Both pandas and polars have a qcut function, so it's just a matter of deciding where to put it.

@rcap107
Copy link
Contributor
rcap107 commented Jun 12, 2025

Actually, doesn't the KBinsDiscretizer already do most of what this feature needs?

@Vincent-Maladiere
Copy link
Member

Actually, doesn't the KBinsDiscretizer already do most of what this feature needs?

Yes indeed! The only drawback I see is that the KBinsDiscretizer converts Pandas and Polars dataframes into NumPy arrays, which creates a copy and has a larger memory footprint. @GaelVaroquaux, should we make our tiny KBinsDiscretizer with skrub's sauce?

@GaelVaroquaux
Copy link
Member
GaelVaroquaux commented Jun 13, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@rcap107 rcap107

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@coconutattitude @Vincent-Maladiere @jeromedockes @rcap107 @GaelVaroquaux
0