-
-
Notifications
You must be signed in to change notification settings - Fork 26.2k
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
base: main
Are you sure you want to change the base?
Conversation
TODO:
|
There was a problem hiding this 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
Thanks for the feedback! - the link to |
+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. |
The docs have been built. Gaël's feedback was added. |
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! |
There was a problem hiding this 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.
@glemaitre I think I addressed all your comments. |
There was a problem hiding this 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.
There was a problem hiding this 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.
doc/whats_new/upcoming_changes/sklearn.utils/31564.enhancement.rst
Outdated
Show resolved
Hide resolved
`table td`is set in notebook with right text-align. | ||
We need to overwrite it. | ||
*/ | ||
.estimator-table table td.param { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. I must be missing something.
Could you share the screenshot of your whole column please?
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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? |
Oh right my bad I forgot about that.
yes please, sorry for the misleading request |
Don't worry. Be happy. 🙂 |
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). |
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By :user:`Dea María Léon <DeaMariaLeon>` | |
By :user:`Dea María Léon <DeaMariaLeon>`. |
There was a problem hiding this comment.
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)
sklearn/utils/_repr_html/params.css
Outdated
/* Hide by default */ | ||
opacity: 0; | ||
visibility: hidden; | ||
pointer-events: none; |
There was a problem hiding this comment.
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):

def test_generate_link_to_param_doc_special_characters(): | ||
class MockEstimator: | ||
"""Mock class | ||
param with space : {'stay', 'leave'} default='don't know' |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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. I must be missing something.
Could you share the screenshot of your whole column please?
I'm not sure if you will see my comment. You asked me to add the I think I understood what you mean with the padding, but I'm not 100% sure. |
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