Skip to content

MNT Change print_changed_only default to True#17061

Merged
NicolasHug merged 10 commits into
scikit-learn:masterfrom
NicolasHug:changed_only_default
Apr 27, 2020
Merged

MNT Change print_changed_only default to True#17061
NicolasHug merged 10 commits into
scikit-learn:masterfrom
NicolasHug:changed_only_default

Conversation

@NicolasHug

@NicolasHug NicolasHug commented Apr 27, 2020

Copy link
Copy Markdown
Member

This PR changes print_changed_only=True as the default

Would be nice in 0.23 but not a priority either

CC @jnothman @adrinjalali @thomasjpfan @amueller @glemaitre

@NicolasHug NicolasHug changed the title [WIP] Change print_changed_only default to True [MRG] Change print_changed_only default to True Apr 27, 2020
Comment thread sklearn/utils/tests/conftest.py Outdated

@rth rth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! I think it would be nice to have in this release!

Comment thread sklearn/utils/tests/test_pprint.py Outdated

@ogrisel ogrisel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with some comments:

Comment thread sklearn/utils/tests/conftest.py Outdated
Comment thread doc/whats_new/v0.23.rst Outdated
@NicolasHug

Copy link
Copy Markdown
Member Author

Thanks for the reviews! I addressed the comments

Maybe we should wait for @jnothman before merging since I believe he had some comments about this

@thomasjpfan thomasjpfan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am happy with the code quality. I agree we should wait for @jnothman

Comment thread doc/whats_new/v0.23.rst Outdated
printing an estimator. You can restore the previous behaviour by using
`sklearn.set_config(print_changed_only=False)`. Also, note that it is
always possible to quickly inspect the parameters of any estimator using
`est.get_params()`. :pr:`17061` by `Nicolas Hug`_.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I was looking to see if you had these docs. Nicely done. I'd consider noting deep=False to not overwhelm uses.

@NicolasHug

Copy link
Copy Markdown
Member Author

Thanks all for the reviews, merging

@NicolasHug NicolasHug changed the title [MRG] Change print_changed_only default to True MNT Change print_changed_only default to True Apr 27, 2020
@NicolasHug NicolasHug merged commit 964c830 into scikit-learn:master Apr 27, 2020
@NicolasHug NicolasHug deleted the changed_only_default branch April 27, 2020 22:22
@NicolasHug

Copy link
Copy Markdown
Member Author

ping #17010

gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020
santisoler added a commit to fatiando/verde that referenced this pull request Jun 3, 2020
Change the expected representation of any gridder based on the new
default behaviour of Scikit Learn. After 0.23, Scikit Learn only shows
those parameters whose default value has been changed when giving
a string representation of the gridder. See
scikit-learn/scikit-learn#17061 and its changelog for more information.

Fix #266
santisoler added a commit to fatiando/verde that referenced this pull request Jun 3, 2020
Change the expected representation of any gridder based on the new
default behaviour of Scikit Learn. After 0.23, Scikit Learn only shows
those parameters whose default value has been changed when giving
a string representation of the gridder. See
scikit-learn/scikit-learn#17061 and its changelog for more information.
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants