-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
ENH: Display parameters in HTML representation #30763
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
Conversation
This looks super cool! What about a more descriptive title for the PR? Every time I see a notification related to it pop up I think: what Maybe something like "Display parameters in HTML representation"? |
Hey folks, I am so happy to see more accessibility discussions in the wild ✨💜 Thank you for being thoughtful and striving to serve our community better. If you wanted to make the style more in line with the PyData Sphinx Theme:
The reason I bring these resources is per @betatim 's comment:
We have spent much time thinking about accessibility and design for PyData Sphinx Theme and documenting our guidelines so our community can reuse and build on them without reinventing the wheel. |
Here is the example again: Column Transformer with Mixed Types |
Sorry for jumping in. I don't want to bring noise to a PR that already has 6 participants. I just wanted to share that I had a look on the render linked in #30763 (comment) and I noticed a change in behavior. Personally I find it more useful to know the column names than knowing they were selected using a |
@ArturoAmorQ thanks for bringing it up!.. it's a bug.. |
I don't think that there is a bug here. @ArturoAmorQ I think that you click on the first pipeline in ![]() I was a bit surprise because the code never execute any function when it comes to diagram. However, it could be a further improvement potentially by recording the column name once |
Actually it's me who read the example too fast. Sorry for that. There are 2 pipelines in the example and the second one uses the Still I wonder if we can somehow retrieve the column names from the Sorry again for the noise. |
Thanks @trallard for the guideline. We definitely need some visual improvements with those diagrams. Basically, this PR was a good starting point to find out a couple of limitations: inconsistency between IDEs/doc rendering and dark/light theme management (among others). We should probably limit the scope of this PR but we need to revisit specifically the look of those diagrams and those guidelines will be useful for sure. We can hopefully also reuse knowledge that we gained when developing a couple of add-ons in |
+1 on limiting scope. |
@trallard , happy to have you around. Thanks a lot! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rendering looks fine to me (I no longer have issues with the contrast).
I do have one question on the code (inlined in the review).
In addition, I just wanted to check: this refactor does not change the protocol for subclasses in other packages compared to the one that we had before? If it does, we should document this clearly.
return self._repr_html_inner | ||
|
||
def _repr_html_inner(self): | ||
return self._html_repr() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this method? It seems to me that we could simply call _html_repr istead of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was taken from the original base.py
code (that's the way it is in main
). But I forgot to add its docstring:
This function is returned by the @property `_repr_html_` to make
`hasattr(estimator, "_repr_html_") return `True` or `False` depending
on `get_config()["display"]`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I should do it differently of course I'll do it, but in the meantime I added the docstring.
No, it doesn't. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @GaelVaroquaux for the review. Looks good on my side. Let's merge.
Thanks @DeaMariaLeon it is a nice improvement.
Thank you All! |
Great work! Thanks a lot!!
|
Whoop whoop 🥳 |
I think that this should be a highlight of the release, by the way
|
@betatim The feature is particularly useful when you have nested pipeline and column transformer to get the fully qualified name, e.g. We started only by copying the parameter name but I don't mind extending to the parameter value if we think it is useful. |
That makes sense. Maybe something to show off in the release highlight to help others realise why copying the name is useful |
Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>
Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>
Reference Issues/PRs
Working on first point on #26595
Fixes: #21266
What does this implement/fix? Explain your changes.
Any other comments?
_repr_mimebundle_
@glemaitre : WDYT?