[go: nahoru, domu]

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

API Config: change default display to "diagram" #22856

Merged
merged 6 commits into from
Mar 21, 2022

Conversation

jeremiedbb
Copy link
Member

Fixes #21285

@jeremiedbb jeremiedbb added this to the 1.1 milestone Mar 15, 2022
@jeremiedbb jeremiedbb changed the title API Change default display to "diagram" API Config: change default display to "diagram" Mar 15, 2022
doc/modules/compose.rst Outdated Show resolved Hide resolved
@ogrisel
Copy link
Member
ogrisel commented Mar 21, 2022

@jeremiedbb
Copy link
Member Author

Actually it is, you're missing the # doctest: +SKIP :)

@ogrisel
Copy link
Member
ogrisel commented Mar 21, 2022

Actually it is, you're missing the # doctest: +SKIP :)

Indeed. I forgot that the code of code snippets in the user guide is not executed by sphinx as done in the examples... We already have the problem on the dev site:

https://scikit-learn.org/dev/modules/compose.html#visualizing-composite-estimators

so this is not a problem of this PR.

Copy link
Member
@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing this.

import sklearn

sklearn.set_config(display="diagram")

Copy link
Member

Choose a reason for hiding this comment

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

Note: it seems that we actually do not display the pipeline diagram in this example but I think this is fine.

@ogrisel ogrisel merged commit bb5ab53 into scikit-learn:main Mar 21, 2022
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Apr 6, 2022
sebp added a commit to sebp/scikit-survival that referenced this pull request Aug 6, 2022
sebp added a commit to sebp/scikit-survival that referenced this pull request Aug 7, 2022
sebp added a commit to sebp/scikit-survival that referenced this pull request Aug 13, 2022
sebp added a commit to sebp/scikit-survival that referenced this pull request Aug 13, 2022
@dkgaraujo
Copy link

Hi all,
just wanted to share an experience with the API change. It was solved after I included the following language at the beginning of the notebook, but I think it would be useful for others to be aware of error messages that arise as a result of this change (I believe).

After I updated from v1.0.2 to v1.1.2, a normal notebook calling a sklearn.ensemble.RandomForestRegression object would yield the following error in this actions result.

I then included the language as follows at the beginning of notebooks, and it did improve.

from sklearn import set_config
set_config(display='text')

So long story short is that this change in the default options might introduce some hard-to-debut errors, such as in this case a TypeError.

@thomasjpfan
Copy link
Member

The issue comes from RegrsesionBenchmark.get_params returning estimator classes as values. I opened #24512 to support this use case. For RegrsesionBenchmark, it will look like this:

Screen Shot 2022-09-26 at 9 57 16 AM

@dkgaraujo
Copy link

Many thanks for this fast iteration - it will be very welcome indeed if this use case is supported! Not only it would align with the other use cases, but it also would take away the need to re-adjust the parameter to its older default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC Enable set_config(display='diagram') by default?
5 participants