-
Notifications
You must be signed in to change notification settings - Fork 258
ENH - adding doc link to html repr of estimators #2036
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?
Changes from all commits
5fcf629
baf63fb
0fde8fd
e40ecfa
3156828
8213776
4a41b3f
b3403c0
ade23b8
fe0fc13
7f774ea
cb48d16
c2eb8b5
1f1a828
79cc5e9
48949d2
f1f07d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| from sklearn.base import BaseEstimator | ||
|
|
||
|
|
||
| class BaseTransformer(BaseEstimator): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can call it SkrubBaseEstimator instead because the point is not transformer vs estimator, but that it should point to the skrub documentation. it could also be SkrubEstimator but that is too similar to SkrubLearner
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also could you add a docstring to this class to say it is a base class for stuff shared by all estimators defined in skrub, which at the moment is only the documentation url |
||
| _doc_link_module = "skrub" | ||
|
|
||
| # Defining this as a property because it inherits from _HTMLDocumentationLinkMixin, | ||
| # which also defines _doc_link_template as a property, and we want to be able | ||
| # to override it. | ||
| @property | ||
| def _doc_link_template(self): | ||
| return getattr( | ||
| self, | ||
| "__doc_link_template", | ||
| "https://skrub-data.org/stable/reference/generated/" | ||
| "{estimator_module}.{estimator_name}.html", | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -787,6 +787,17 @@ def describe_params(self): | |
| """ | ||
| return describe_params(eval_choices(self.data_op), choice_graph(self.data_op)) | ||
|
|
||
| _doc_link_module = "skrub" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here also we could remove this and replace BaseEstimator by SkrubBaseEstimator as the base class right? also as the base class of _BaseParamSearch |
||
|
|
||
| @property | ||
| def _doc_link_template(self): | ||
| return getattr( | ||
| self, | ||
| "__doc_link_template", | ||
| "https://skrub-data.org/stable/reference/generated/" | ||
| "{estimator_module}.{estimator_name}.html", | ||
| ) | ||
|
|
||
|
|
||
| def _to_Xy_pipeline(learner, environment): | ||
| return learner.__skrub_to_Xy_pipeline__(environment) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| import re | ||
|
|
||
| from sklearn.utils import estimator_html_repr | ||
|
|
||
| from skrub import ( | ||
| ApplyToCols, | ||
| Cleaner, | ||
| DropCols, | ||
| SelectCols, | ||
| StringEncoder, | ||
| TableVectorizer, | ||
| ) | ||
|
|
||
|
|
||
| def test_doc_link_apply_to_cols(): | ||
| """The wrapped transformer's doc link appears in the HTML repr of ApplyToCols.""" | ||
| html = estimator_html_repr(ApplyToCols(StringEncoder())) | ||
| links = set(re.findall(r'href="(https?://[^#"]+)"', html)) | ||
| assert ( | ||
| "https://skrub-data.org/stable/reference/generated/skrub.ApplyToCols.html" | ||
| in links | ||
| ) | ||
|
|
||
| html = estimator_html_repr(ApplyToCols(TableVectorizer())) | ||
| links = set(re.findall(r'href="(https?://[^#"]+)"', html)) | ||
| assert ( | ||
| "https://skrub-data.org/stable/reference/generated/skrub.TableVectorizer.html" | ||
| in links | ||
| ) | ||
|
|
||
|
|
||
| def test_doc_link_skrub_class_select_cols(): | ||
| """Public skrub classes get a link to skrub documentation.""" | ||
| link = SelectCols(cols=[])._get_doc_link() | ||
| assert link == ( | ||
| "https://skrub-data.org/stable/reference/generated/skrub.SelectCols.html" | ||
| ) | ||
| link = DropCols(cols=[])._get_doc_link() | ||
| assert link == ( | ||
| "https://skrub-data.org/stable/reference/generated/skrub.DropCols.html" | ||
| ) | ||
|
|
||
|
|
||
| def test_doc_link_table_vectorizer(): | ||
| """Public skrub classes get a link to skrub documentation.""" | ||
| link = TableVectorizer()._get_doc_link() | ||
| assert link == ( | ||
| "https://skrub-data.org/stable/reference/generated/skrub.TableVectorizer.html" | ||
| ) | ||
| link = Cleaner()._get_doc_link() | ||
| assert link == ( | ||
| "https://skrub-data.org/stable/reference/generated/skrub.Cleaner.html" | ||
| ) |
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'm not sure i understood this comment, but also the scikit-learn diagram machinery is quite complicated so maybe it's not easy to explain in a short comment