-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
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
base: main
Are you sure you want to change the base?
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"? |
@GaelVaroquaux from the rendered docs: Column Transformer with Mixed Types However the copy-paste button is not shown there. It works on vscode and jupyter. @glemaitre is this normal? (that here in the CI this is not shown). |
Oups, I might have added a bug let me quickly check. |
OK so now, I see two issues:
I'll make a review with those improvements. |
It looks really good! I would make the font of the parameter table a bit darker, as right now the contrast is weak and thus it compromises readability (in particular given that the font is small) |
Good point regarding the contrast. At first, we used the contrast to distinguish the default from non-default but by using the color, we can therefore used dark color. I made a PR against your PR regarding the icon. I also think that we can use a color for the table that does not adapt with the background as it is problematic on sphinx-gallery. At the end, the table would be always on the blue (fitted) or orange (unfitted) background and we therefore make it fix whether we are in dark mode or not. |
ENH improve slightly rendering of the parameters table
Drive by comment about contrast and readability: there are tools like Google Lighthouse and something from Microsoft that I forgot the name of that will tell you if the contrast between font colour and background is high enough according to WCAG (the standard on accessibility?!). I'd use those tools to define "high enough contrast" and maybe also see if WCAG (or something like https://getbootstrap.com/) has a recommendation for how to highlight default vs not default. There was a great talk at PyCon Germany about the work that went into pydata-sphinx-theme to make it more accessible and one of my main takeaways from it was that accessibility is a bit like cryptography: so many things to think about that it is worth not rolling your own :D |
Thanks Tim. That reminds me that I had watched one on youTube about accessibility. Maybe it's the same one!.. I'll try to actually use them now. 🤓 edit: I'll still check the ones Tim mentioned. |
Checking the accessibility of the column transformer example on the main branch, there are 116 contrast errors. But (if this is accurate) they are on the code. In case there is interest, one can see where those are: https://wave.webaim.org/report#/https://scikit-learn.org/stable/auto_examples/compose/plot_column_transformer_mixed_types.html |
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! |
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?