Repository navigation
ENH improve visual HTML estimator representation - #26616
Conversation
Add a hover label
Better CSS for doc links
More CSS improvements
GaelVaroquaux
left a comment
GaelVaroquaux
left a comment
There was a problem hiding this comment.
A review, with comments on the tests, @Ines1999
|
|
||
| pipeline5 = Pipeline([ | ||
| ('classifier', stacking_classifier) | ||
| ]) |
There was a problem hiding this comment.
I'm not sure that all these objects are important. We just need one that creates a _VisualBlock
glemaitre
left a comment
glemaitre
left a comment
There was a problem hiding this comment.
I put this PR "ready for review".
In short, I fixed the dark/light mode. I put everything inside a mixin such that it is well tested. I made the way to retrieve the documentation more flexible (providing a callable) in case that what we have in scikit-learn would not be enough (I am thinking ahead in imbalanced-learn. I added the documentation in a specific section with a warning since it is all private for the moment and should be considered experimental.
In a future PR, I think that we should have a section in the User Guide describing how to interact with these diagram/representation. Currently we have a section in the composition section but this is kind of hidden. I am thinking that we should go back to the visualization section and introduce the tool there.
|
pinging @GaelVaroquaux @betatim to finalize this one ;) |
glemaitre
left a comment
glemaitre
left a comment
There was a problem hiding this comment.
Approving myself as well :)
|
Should I review this (ie: is my review acceptable, or am I tainted by the fact that I contributed to this PR?) |
Yes, please. The PR is ready for review. |
|
Looks beautiful to me (this is how it renders under Jupyter, firefox, ubuntu): |
|
Be aware that I included the feature shown in #27637 The main color background and font color should adapt themselves in notebook and vs-code:
For other IDE, it will fallback to the current OS-specific settings. This can be improved in the future. |
Co-authored-by: Tim Head <betatim@gmail.com>
|
Are there still pending comments from Tim to be addressed? Having a look at the PR it seems to me that it's the case, but maybe I am misreading this. |
|
I normally addressed all @betatim comments. |
|
@betatim : this is good with me. |
|
The future will be colourful! Unless you are using dark mode in which case it'll be a bit more muted :D :D Thanks a lot for this cool work! |
Co-authored-by: ines.ibnukhsein <ines.ibnukhsein@dataiku.com> Co-authored-by: Gael Varoquaux <gael.varoquaux@normalesup.org> Co-authored-by: Ines <ines-99@ukr.net> Co-authored-by: Lilian <lilian@boulard.fr> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Loïc Estève <loic.esteve@ymail.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Tim Head <betatim@gmail.com>
|
Darn, the support of the dark mode kinda screwed up the display in the documentation https://scikit-learn.org/dev/auto_examples/miscellaneous/plot_estimator_representation.html: |
|
Feel free to merge #27659 @GaelVaroquaux to fix it :) |
Co-authored-by: ines.ibnukhsein <ines.ibnukhsein@dataiku.com> Co-authored-by: Gael Varoquaux <gael.varoquaux@normalesup.org> Co-authored-by: Ines <ines-99@ukr.net> Co-authored-by: Lilian <lilian@boulard.fr> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Loïc Estève <loic.esteve@ymail.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Tim Head <betatim@gmail.com>
8 participants
Reference Issues/PRs
Addressing task 2 in #26595
What does this implement/fix? Explain your changes.
I am working on linking the html repr of the estimator with the documentation link of the estimator.
For the moment I am moving the css stylesheet to a separate file.
Any other comments?
Not ready for merge, the html will be updated further later.