Skip to content

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: 8184ab5. 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
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.

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

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.

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.

LGTM on my side. It looks like a net improvement.
@jeremiedbb do you want to have a final look for merging the PR.

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.

Here's a new pass. It's great but I think that we can improve the usability a little bit, see my comments below.

`table td`is set in notebook with right text-align.
We need to overwrite it.
*/
.estimator-table table td.param {
Copy link
Member

Choose a reason for hiding this comment

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

The tooltip appears on hovering over the a.param-doc-link element but there's a big padding on td.param, see
param_cell
I wonder if we could get rid of the td.param padding such that the a element takes all the space (probably need to set a margin to keep having some space around the text)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no padding for that - or at least I don't see it. And if one looks at the whole column, it's as wide as the widest parameter name. So loss has an empty space, but could you show the whole column please? Or maybe you could share the code so I can see it locally? thanks

Copy link
Member

Choose a reason for hiding this comment

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

There is still the padding in td.param. It's a lot less problematic now that the tooltip has moved a bit to the right. I wonder however if it's possible to remove the padding on td.param and add a margin on a.param-doc-link instead. Because right now we can hover over some parts off the cell without triggering the apparition of the tooltip which is a bit weird. Not a major issue though.

Copy link
Contributor Author

@DeaMariaLeon DeaMariaLeon Sep 1, 2025

Choose a reason for hiding this comment

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

@jeremiedbb I'm so sorry: I don't see the td.param padding that you mention. Which line do you mean? Did you see my previous comment? params is as wide as it's wider param name. There is no padding for that.
Could you share the screenshot of your whole column please?
I must be missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I think the padding is added by the browser. So I must force it to 0.

def test_generate_link_to_param_doc_special_characters():
class MockEstimator:
"""Mock class
param with space : {'stay', 'leave'} default='don't know'
Copy link
Member

Choose a reason for hiding this comment

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

does such a param exists ? I don't see how a parameter could have a space in it's name in Python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it.
I had added this test when I was struggling with the link generation. Sometimes it had spaces in-between.. but not in the param name. I was looking for a whole line with spaces in between as you had suggested.
Anyway, I deleted it. Thanks.

@DeaMariaLeon
Copy link
Contributor Author

DeaMariaLeon commented Sep 1, 2025

@jeremiedbb it's failing because of "ModuleNotFoundError: No module named 'numpydoc'" - this is after the suggestion here.

I'm not sure what the best way to go is. Should I add the file I removed back? Should this be a dependency?
(I think that now it's only an optional dependency).
I'd be happy to hear your thoughts.

@jeremiedbb
Copy link
Member

(I think that now it's only an optional dependency).

Oh right my bad I forgot about that.

Should I add the file I removed back?

yes please, sorry for the misleading request

@DeaMariaLeon
Copy link
Contributor Author

Don't worry. Be happy. 🙂

@DeaMariaLeon
Copy link
Contributor Author

Friendly ping @jeremiedbb.. because it's green.

FYI I did the last (previous) commit because the parameter's value column was right-aligned (in the ci).

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.

I just have one last comment, everything looks good otherwise.

`table td`is set in notebook with right text-align.
We need to overwrite it.
*/
.estimator-table table td.param {
Copy link
Member

Choose a reason for hiding this comment

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

There is still the padding in td.param. It's a lot less problematic now that the tooltip has moved a bit to the right. I wonder however if it's possible to remove the padding on td.param and add a margin on a.param-doc-link instead. Because right now we can hover over some parts off the cell without triggering the apparition of the tooltip which is a bit weird. Not a major issue though.

more generally of estimators inheriting from :class:`base.BaseEstimator`
now displays the parameter description as a tooltip and has a link to the online
documentation for each parameter.
By :user:`Dea María Léon <DeaMariaLeon>`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
By :user:`Dea María Léon <DeaMariaLeon>`
By :user:`Dea María Léon <DeaMariaLeon>`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Olivier asked me to remove it here #31564 (comment)

/* Hide by default */
opacity: 0;
visibility: hidden;
pointer-events: none;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@glemaitre When visibility is hidden the element stays in the DOM, and it adds extra empty space between the cell where you are, and the next one. That's why I had display:none.
Here are the docs

The empty space (bottom of the cell):

Screenshot 2025-08-21 at 15 40 00

def test_generate_link_to_param_doc_special_characters():
class MockEstimator:
"""Mock class
param with space : {'stay', 'leave'} default='don't know'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it.
I had added this test when I was struggling with the link generation. Sometimes it had spaces in-between.. but not in the param name. I was looking for a whole line with spaces in between as you had suggested.
Anyway, I deleted it. Thanks.

`table td`is set in notebook with right text-align.
We need to overwrite it.
*/
.estimator-table table td.param {
Copy link
Contributor Author

@DeaMariaLeon DeaMariaLeon Sep 1, 2025

Choose a reason for hiding this comment

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

@jeremiedbb I'm so sorry: I don't see the td.param padding that you mention. Which line do you mean? Did you see my previous comment? params is as wide as it's wider param name. There is no padding for that.
Could you share the screenshot of your whole column please?
I must be missing something.

@DeaMariaLeon
Copy link
Contributor Author

I'm not sure if you will see my comment. You asked me to add the . here, but Olivier had asked me to remove it earlier.
By :user:Dea María Léon <DeaMariaLeon>.`

I think I understood what you mean with the padding, but I'm not 100% sure.

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.

5 participants