-
-
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
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 |
@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)
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 see, but all the changelog fragments finish with the dot so it's unlikely that it was the dot causing towncrier issues
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.
That's true, the others have it.
Done.
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. edit: |
I hope I'm not being a nuisance but it's green and I'll be away for a few days. I think I addressed all the feedback @jeremiedbb. |
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.
Sorry I've been fighting against css for a while trying to make the a.param-doc-link take the whole td.param space. I wanted that the tooltip appear when hovering over the whole cell. Unfortunately, I didn't manage to make things like height: 100%
work... I just pushed a commit to add padding on the link itself instead of a margin, and not only horizontal. Now the link takes a good portion of the cell. Still not the whole but good enough for now. I'll open a separate issue to see if can find a solution. But we can merge this PR in the mean time.
LGTM. Thanks @DeaMariaLeon !
Thank you everybody! 🥳 |
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