FEA add link for HTML representation #1051
Conversation
|
if the bug is in scikit-learn and this is not critical, could we just wait for it to be fix in scikit-learn without vendoring the htmlmixin class? also once it is fixed in scikit-learn would we still need to inherit from a private scikit-learn class? |
Yes but we need to bump the minimum version also once it is fixed in scikit-learn would we still need to inherit from a private scikit-learn class? Actually, we don't need to. We only need to set the class attributes. |
This is somewhat unrelated to this PR, but we were thinking with @jeromedockes whether or not we should try to support as many Python and scikit-learn versions as possible, for many potential users are often stuck with an old Python version (e.g. APHP). We can talk about this idea somewhere else, though. |
|
@Vincent-Maladiere I share the same feeling and this is the reason why to vendor for the moment as a fix. |
| @@ -0,0 +1,103 @@ | |||
| import itertools | |||
There was a problem hiding this comment.
could we name this module something more explicit like "_sklearn_html_repr_utils" or similar?
| # TODO: subsequently, we should remove the inheritance from _HTMLDocumentationLinkMixin | ||
| # for each estimator then. | ||
| if sklearn_version > parse_version("1.6"): | ||
| from sklearn.utils._estimator_html_repr import _HTMLDocumentationLinkMixin |
There was a problem hiding this comment.
IIUC we will now have 2 of those in every skrub estimator's parents, one directly and one through the BaseEstimator?
There was a problem hiding this comment.
Yep. The one on the left in the inheritance will be the one used.
|
@glemaitre could you do a so we can see the new TableVectorizer display in the examples? thanks! |
| @@ -101,3 +101,9 @@ def doc_link_url_param_generator(estimator): | |||
| "estimator_module": estimator_module, | |||
| "estimator_name": estimator_name, | |||
| } | |||
|
|
|||
|
|
|||
| class _SkrubHTMLDocumentationLinkMixin(_HTMLDocumentationLinkMixin): | |||
There was a problem hiding this comment.
even when the scikit-learn fix is released, we will most likely want this mixin to avoid repeating these 3 attributes everywhere. so in the end all we will have to remove is the HTMLDocumentationLinkMixin which is confined to this module.
so after all I am +1 for this PR
|
@glemaitre ok, I misread your first message. I'm +0 or +1 as long as we don't need to bump the minimal version. |
This PR add the necessary class and function to add a link to the documentation when showing the HTML representation of an estimator. Unfortunately, there is a bug in scikit-learn that will be corrected in 1.6 and we need to vendor the fix and inherit from the mixin.