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.

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 72 commits into
base: main
Choose a base branch
from

Conversation

DeaMariaLeon
Copy link
Contributor

@DeaMariaLeon DeaMariaLeon commented Jun 17, 2025

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

Copy link

github-actions bot commented Jun 17, 2025

✔️ Linting Passed

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

Generated for commit: ac8c656. 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

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

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?

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.

@DeaMariaLeon
Copy link
Contributor Author

Example here

@ogrisel
Copy link
Member

ogrisel commented Aug 4, 2025

Could you please adjust the CSS to make the parameter docstring preview larger (a bit more than 80 character large) and avoid the centering?

image

@jeremiedbb
Copy link
Member

The idea of a tooltip with the parameter description is interesting. I find the font a bit small though but bigger might make the tooltip take too much space. I also think that it's currently missing the parameter type (i.e. the first line of the param description: name: type).

If we go for the tooltip, then I don't think that a link for each parameter is needed anymore. The link for the estimator is enough then. And the tooltip could be displayed on hovering over the param name cell.

Thought for later: another tooltip could be displayed on hovering over the param value cell to show a complete list of valid types and (ranges of) values, read from the _parameter_constraints dict.

@DeaMariaLeon
Copy link
Contributor Author

DeaMariaLeon commented Aug 6, 2025

I think I added to the tooltip all the feedback, including the two last comments: avoid centring the text, get a wider text area and add params names and types. I also modified the code that was suggested -- to add the tooltip -- as it was "buggy".

Everything works well locally, and I'm able to build the examples that the ci is complaining about. So, not sure what to do next - I pushed a few times to restart the process with no success.

Please note that when I try to build the examples locally following the instructions from the documentation it took more than 6 minutes - and there were many warnings and errors. But I get the same ones building on the main branch. Still, the example gets built.

edit: 6min for one example only. The examples get built here as well - I just noticed. But there is the ci error.

@DeaMariaLeon
Copy link
Contributor Author

Docs build failure unrelated. See #31918 (comment)

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.

4 participants