Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Open
wants to merge 143 commits into
base: main
Choose a base branch
Loading
from

Conversation

DeaMariaLeon
Copy link
Contributor

@DeaMariaLeon DeaMariaLeon commented Feb 3, 2025

Reference Issues/PRs

Working on first point on #26595
Fixes: #21266

What does this implement/fix? Explain your changes.

  • This code allows to visualise the estimator's parameter values. It adds an interactive dropdown hidden by default.
  • Copy paste button added. When clicking on it, the parameter's name is saved to the clipboard.

Any other comments?

@glemaitre : WDYT?

Copy link

github-actions bot commented Feb 3, 2025

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: f0e7224. Link to the linter CI: here

@DeaMariaLeon DeaMariaLeon changed the title WIP: get_params() work - not ready for review WIP: get_params() work Feb 3, 2025
@glemaitre glemaitre self-requested a review February 6, 2025 15:28
@betatim
Copy link
Member

betatim commented Feb 7, 2025

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 get_params is broken??!! To one second later realise what this PR is really about :D

Maybe something like "Display parameters in HTML representation"?

@DeaMariaLeon DeaMariaLeon changed the title WIP: get_params() work WIP: Display parameters in HTML representation Feb 7, 2025
@DeaMariaLeon
Copy link
Contributor Author

@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).

@glemaitre
Copy link
Member

Oups, I might have added a bug let me quickly check.

@glemaitre
Copy link
Member

OK so now, I see two issues:

  • it looks like the font-awesome download does not work as expected. I'll find a work-around
  • I review in light mode and I see that we have to improve the dark mode side and as well the fitted one.

I'll make a review with those improvements.

@glemaitre glemaitre self-requested a review May 12, 2025 15:31
@GaelVaroquaux
Copy link
Member

from the rendered docs: Column Transformer with Mixed Types

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)
image

@glemaitre
Copy link
Member

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.

@betatim
Copy link
Member

betatim commented May 13, 2025

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

@DeaMariaLeon
Copy link
Contributor Author

DeaMariaLeon commented May 13, 2025

Thanks Tim. That reminds me that I had watched one on youTube about accessibility. Maybe it's the same one!..
It was at PyConDE by Tania Allard (@trallard). All the credits for her:

Accessibility-websites

I'll try to actually use them now. 🤓

edit: I'll still check the ones Tim mentioned.

@glemaitre
Copy link
Member

So here are two screenshot. The inner table will not try to change its background depending on the light/dark theme and we improve the contrast.

image

image

@DeaMariaLeon
Copy link
Contributor Author

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

@trallard
Copy link

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.
There was also another talk at PyCon DE this year by Smera, our fabulous designer!

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:

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

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.
So, you might find some of our guidelines and styles helpful for you moving forward.
Alternatively, if you need or would like accessibility-related feedback or brainstorming, feel free to contact me, and I will be more than happy to help.

@DeaMariaLeon
Copy link
Contributor Author

Here is the example again: Column Transformer with Mixed Types

@ArturoAmorQ
Copy link
Member

ArturoAmorQ commented May 13, 2025

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.

  • On main:
    image
  • This PR:
    image

Personally I find it more useful to know the column names than knowing they were selected using a make_column_selector object. Is there a way to retrieve the actual names of the selected columns from the object?

@DeaMariaLeon
Copy link
Contributor Author

DeaMariaLeon commented May 13, 2025

@ArturoAmorQ thanks for bringing it up!.. it's a bug..
The names of the columns are shown correctly before make_column_selector is used (the first diagram in the document is fine).

@glemaitre
Copy link
Member

I don't think that there is a bug here. @ArturoAmorQ I think that you click on the first pipeline in main and the second pipeline in this PR. In main, it is the rendering of the the second pipeline:

image

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 fit is called and show them once the pipeline is fitted. Before fit, we cannot do better than the signature of the object. We could also the representation of the make_column_selector function to have a better string reprenstation.

@ArturoAmorQ
Copy link
Member

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 make_column_selector. Your PR has the same behavior as before.

Still I wonder if we can somehow retrieve the column names from the make_column_selector 🤔 In any case that would be for a different PR.

Sorry again for the noise.

@glemaitre
Copy link
Member

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 skrub.

@trallard
Copy link

+1 on limiting scope.
If there is anything I can help with just give me a ping whenever.

@GaelVaroquaux
Copy link
Member

@trallard , happy to have you around. Thanks a lot!

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.

Display all parameter values in a tabular for a tab of the notebook HTML repr of estimators
7 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.