Skip to content

ENH Adding a column to link each parameter docstring row in params table display #31564

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 99 commits into
base: main
Choose a base branch
from
Open

ENH Adding a column to link each parameter docstring row in params table display #31564

wants to merge 99 commits into from
+1,078 −27

Conversation

DeaMariaLeon
Copy link
Contributor

@DeaMariaLeon DeaMariaLeon commented Jun 17, 2025 •
edited
Loading

Reference Issues/PRs

Adds a column with a link to each parameter docstring in the params table (in the HTML display).
This was discussed while working on the PR that added the table.

"Text fragments" code shared by @ogrisel here :
#30763 (comment)

What does this implement/fix? Explain your changes.

Added a column to the table with the new link
Modified ParamsDict to include the estimator_type. After several tries it was the only way I found at the moment.

Any other comments?

@glemaitre

Screenshot 2025-06-18 at 11 47 55

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 17, 2025 •
edited
Loading

✔️ Linting Passed

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

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

@glemaitre glemaitre self-requested a review June 17, 2025 21:20
@DeaMariaLeon
Copy link
Contributor Author

DeaMariaLeon commented Jun 18, 2025 •
edited
Loading

Example here

TODO:

  1. Address Olivier's comment (during a call). He mentioned that the ReprHTMLMixin could be set-up in a way that we only add documentation link of estimators that have similar HTML structure.
  2. Test for broken links
  3. The "?" link is too small, but it's the same size of the text. latest doc built - It seems that having the same text for each hyperlink is different than the recommended "best practices".

@DeaMariaLeon DeaMariaLeon marked this pull request as ready for review June 20, 2025 12:25
@DeaMariaLeon DeaMariaLeon changed the title WIP: ENH Adding a column to link each parameter docstring row in params table display ENH Adding a column to link each parameter docstring row in params table display Jun 20, 2025
DeaMariaLeon
DeaMariaLeon commented Jul 7, 2025
View reviewed changes
sklearn/utils/_repr_html/params.py
@@ -23,6 +76,31 @@ def _read_params(name, value, non_default_params):
return {"param_type": param_type, "param_name": name, "param_value": cleaned_value}

Copy link
Contributor Author

@DeaMariaLeon DeaMariaLeon Jul 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It seems that r.maxstringis not working.
Noticed while working on adding methods table.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe open a dedicated issue with a reproducer?

jeremiedbb
jeremiedbb reviewed Jul 21, 2025
View reviewed changes
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @DeaMariaLeon.

I tested on the example you linked and it worked fine except for the C parameter of LogisticRegression. Maybe the text fragment requires more than 1 character ?

Besides that, here are some comments

@DeaMariaLeon
Copy link
Contributor Author

tested on the example you linked and it worked fine except for the C parameter of LogisticRegression. Maybe the text fragment requires more than 1 character ?

Thanks for the feedback! - the link to C that you mention works for me locally. But the link to my example doesn't work (the whole example, not the parameter link). Maybe on the next doc build I can see it.. ?
I addressed some of the comments you left. I'll work on the rest.

glemaitre
glemaitre reviewed Aug 25, 2025
View reviewed changes
@glemaitre glemaitre self-requested a review August 25, 2025 08:32
glemaitre
glemaitre reviewed Aug 25, 2025
View reviewed changes
@DeaMariaLeon
Copy link
Contributor Author

Docs have been built if someone wants to try again.

@glemaitre
Copy link
Member

The rendering looks good to me now. @jeremiedbb you can have another look.

I'll go through the tests once more.

@GaelVaroquaux
Copy link
Member

After changes requested by Guillaume example Screenshot 2025-08-20 at 13 38 39

This is soooo cool!!

@GaelVaroquaux
Copy link
Member

One comment: can we (should we), add "display: block;" to "a.param-doc-link, a.param-doc-link:link, a.param-doc-link:visited" so that its hovering on the cell rather than on the text that triggers the tooltip?
I find it more pleasant from an interaction standpoint.

It probably means that we should turn off the dashed underline on the text, because with "display: block" extends the underline too far out.

@DeaMariaLeon
Copy link
Contributor Author

DeaMariaLeon commented Aug 25, 2025 •
edited
Loading

If we remove the dashed underline, I wonder how we should show that there is a link to the actual documentation on the website. "Click to see the docs" at the end of the tooltip doc maybe?

edit: We could keep a dashed line...
@GaelVaroquaux
Screenshot 2025-08-25 at 15 41 32

@GaelVaroquaux
Copy link
Member

edit: We could keep a dashed line...

Very nice!!

@glemaitre
Copy link
Member

I'll like to keep the underline to alert users that there is something special about the this section of the diagram. I would still expect the text to have the hyperlink towards the documentation. Regarding text vs. cell, I don't have a strong opinion (maybe a slight one for the text since I find it easier to disable the tooltip by moving away from the string but it is rather subjective so using the cell is fine as well).

@ogrisel
Copy link
Member

ogrisel commented Aug 25, 2025

I'm not sure that linking to the full doc page is necessary then but why not if you want to have the rst properly rendered.

+1 for keeping the link. Sometimes the rendering can be helpful (e.g. math). Furthermore, when browsing from a mobile device without a mouse, there is no way to get the tooltip so better keep a "click to navigate to doc" interaction in addition to the tooltip.

@DeaMariaLeon
Copy link
Contributor Author

The docs have been built. Gaël's feedback was added.
Example here

@GaelVaroquaux
Copy link
Member

I haven't reviewed the code, but from a user perspective, I think that it looks absolutely great, and it's definitely mergeable for me!! Thanks!

glemaitre
glemaitre reviewed Aug 26, 2025
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

OK here is a pass on the code more in details. I propose some slight changes.

@DeaMariaLeon
Copy link
Contributor Author

@glemaitre I think I addressed all your comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel left review comments

@glemaitre glemaitre glemaitre left review comments

@jeremiedbb jeremiedbb jeremiedbb left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
module:utils
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DeaMariaLeon @jeremiedbb @glemaitre @ogrisel @GaelVaroquaux