Skip to content

scikit-learn compatibility API #460

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
Closed
2 of 3 tasks
glemaitre opened this issue Aug 27, 2018 · 1 comment · Fixed by #462
Closed
2 of 3 tasks

scikit-learn compatibility API #460

glemaitre opened this issue Aug 27, 2018 · 1 comment · Fixed by #462
Assignees
@glemaitre
Labels
For: API Status: Blocker

Comments

@glemaitre
Copy link
Member

glemaitre commented Aug 27, 2018 •
edited
Loading

We need to address the following API issues raised by @jnothman:

  • fit_sample is not a great name, as I have argued elsewhere ;)
  • The fact that imblearn.pipeline.Pipeline has a method called sample that fits its constituents (using fit_sample) rings loud alarm bells. IMO, the sample method should not exist.
  • fit_sample needs, eventually, to be able to update things other than X and y (e.g. weights).
@glemaitre glemaitre self-assigned this Aug 27, 2018
@chkoar
Copy link
Member

chkoar commented Aug 27, 2018

fit_sample is not a great name, as I have argued elsewhere ;)

This is debatable. fit_resample I suppose that could have been better choice. We could change the name of the method and keep an alias for backwards compatibility.

The fact that imblearn.pipeline.Pipeline has a method called sample that fits its constituents (using fit_sample) rings loud alarm bells. IMO, the sample method should not exist.

True. I have thought about that many times.

fit_sample needs, eventually, to be able to update things other than X and y (e.g. weights).

See #457

This was referenced Aug 27, 2018
Merged
Open
massich pushed a commit that referenced this issue Aug 28, 2018
@glemaitre @massich
[MRG+1] EHN: scikit-learn API transition towards fit_resample (#462)
e49c30a 
closes #460 

This PR implements:

- [x] Removing `sample`.
- [x] Having a single `fit_resample`. In addition, we kept an alias `fit_sample` for backcompatibility.
@glemaitre glemaitre reopened this Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@glemaitre glemaitre

Labels
For: API Status: Blocker
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[MRG+1] EHN: scikit-learn API transition glemaitre/imbalanced-learn
2 participants
@glemaitre @chkoar