Skip to content
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.

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

[MRG] API kwonly for utils #17046

Merged
merged 8 commits into from Apr 27, 2020
Merged

[MRG] API kwonly for utils #17046

merged 8 commits into from Apr 27, 2020

Conversation

NicolasHug
Copy link
Member

Towards #15005

@NicolasHug NicolasHug changed the title [WIP] API kwonly for utils [MRG] API kwonly for utils Apr 26, 2020
@NicolasHug
Copy link
Member Author

@adrinjalali @rth this one is the same as #17007 which you approved before, up to some fixes that weren't caught before

API-wise it's strictly the same

rth
rth approved these changes Apr 26, 2020
View reviewed changes
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think we should allow positional accept_sparse in check array to avoid breaking contrib projects too much. See how many time you had to change it in scikit-learn, and each of those was clear enough to pass through review.

Otherwise LGTM.

@NicolasHug
Copy link
Member Author

NicolasHug commented Apr 26, 2020 •
edited

Thanks for the review

OK, it's allowed to be positional

(If you're OK I'll leave the changes as-is though, i.e. using accept_sparse=...)

@rth
Copy link
Member

rth commented Apr 26, 2020

(If you're OK I'll leave the changes as-is though, i.e. using accept_sparse=...)

Yes, sure it's fine. Thanks!

@NicolasHug NicolasHug added this to the 0.23 milestone Apr 27, 2020
@NicolasHug
Copy link
Member Author

easy one @thomasjpfan @adrinjalali @glemaitre ?

adrinjalali
adrinjalali approved these changes Apr 27, 2020
View reviewed changes
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @NicolasHug

@adrinjalali adrinjalali merged commit 76ef8b0 into scikit-learn:master Apr 27, 2020
gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020
@NicolasHug @gio8tisu
API kwonly for utils (scikit-learn#17046)
114da52 
* kwonly for utils

* More

* fixed some

* some more

* iwannagohomepls

* accept_sparse not kwonly anymore
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
@NicolasHug @viclafargue
API kwonly for utils (scikit-learn#17046)
feff0a7 
* kwonly for utils

* More

* fixed some

* some more

* iwannagohomepls

* accept_sparse not kwonly anymore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rth rth rth approved these changes

@adrinjalali adrinjalali adrinjalali approved these changes

Assignees
No one assigned
Labels
Blocker module:utils
Projects
None yet
Milestone
0.23
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@NicolasHug @rth @adrinjalali