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

[MRG] Add missing entries to whatsnew #15398

Merged
merged 8 commits into from Nov 4, 2019

Conversation

NicolasHug
Copy link
Member

Addresses #15384

@NicolasHug NicolasHug added this to the 0.22 milestone Oct 29, 2019
@NicolasHug
Copy link
Member Author

I'll make another PR to clean the whatsnew, I feel like if I do it here there will always be merge conflicts

@thomasjpfan @glemaitre @adrinjalali @jnothman for a quick review?

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

doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
@NicolasHug
Copy link
Member Author

@adrinjalali just added some details about the deprecations and the FutureWarning thing

doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
Comment on lines 26 to 31
Whether a tool is private or public depends on whether you can import it
without a leading underscore in the import path. For example
``sklearn.pipeline.make_pipeline`` is public, while
`sklearn.pipeline._name_estimators` is private.
``sklearn.ensemble._gb.BaseEnsemble`` is private too because the whole `_gb`
module is private.
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to also mention the classes/functions which are private because their name has a leading underscore, eventhough the path may be public.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is sklearn.pipeline._name_estimators, were you thinking of something else?

Copy link
Member

Choose a reason for hiding this comment

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

some _BaseBlahBlah I guess. I think most of them are gonna be in the _filename locations and don't matter anyway. But not for the ones in the root directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry I'm not sure I follow. Why doesn't sklearn.pipeline._name_estimators qualify as "classes/functions which are private because their name has a leading underscore eventhough the path may be public"?

doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
deprecations, `as recommended by the Python documentation
<https://docs.python.org/3/library/exceptions.html#FutureWarning>`_.
``FutureWarnings`` are always shown by default by Python, so the custom
filter has been removed and scikit-learn no longer hinders with user
Copy link
Member

Choose a reason for hiding this comment

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

logger.addHandler(logging.StreamHandler())
is still there, but should be addressed in #15386

Copy link
Member Author

Choose a reason for hiding this comment

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

This section is about warnings. I haven't looked into the logger issue but feel free to edit to your liking!

doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
@NicolasHug
Copy link
Member Author

2 approvals but maybe @rth @jnothman want to give it a last look before merging?

@jnothman
Copy link
Member

jnothman commented Nov 4, 2019

Nice work

@NicolasHug
Copy link
Member Author

Thanks for the reviews, merging

@NicolasHug NicolasHug merged commit 005513a into scikit-learn:master Nov 4, 2019
@jnothman jnothman mentioned this pull request Nov 5, 2019
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants