-
-
Notifications
You must be signed in to change notification settings - Fork 26.1k
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:
|
@@ -23,6 +76,31 @@ def _read_params(name, value, non_default_params): | |||
return {"param_type": param_type, "param_name": name, "param_value": cleaned_value} | |||
|
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.
It seems that r.maxstring
is not working.
Noticed while working on adding methods table.
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.
Maybe open a dedicated issue with a reproducer?
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 |
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: 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 |
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. |
This reverts commit 1fa5d99.
Docs build failure unrelated. See #31918 (comment) |
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