-
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
Changes from 22 commits
5fcf629
baf63fb
0fde8fd
e40ecfa
3156828
8213776
4a41b3f
b3403c0
ade23b8
fe0fc13
7f774ea
cb48d16
c2eb8b5
1f1a828
79cc5e9
48949d2
f1f07d9
ea59f93
a1f520f
20702bd
ce9dc05
0fe8479
1752ffa
e4458b4
d601e99
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 |
|---|---|---|
| @@ -1,11 +1,12 @@ | ||
| import itertools | ||
|
|
||
| from joblib import Parallel, delayed | ||
| from sklearn.base import BaseEstimator, TransformerMixin, clone | ||
| from sklearn.base import TransformerMixin, clone | ||
| from sklearn.utils.validation import check_is_fitted | ||
|
|
||
| from . import _dataframe as sbd | ||
| from . import _utils, selectors | ||
| from ._base import SkrubBaseTransformer | ||
| from ._join_utils import pick_column_names | ||
| from ._single_column_transformer import RejectColumn, is_single_column_transformer | ||
|
|
||
|
|
@@ -15,7 +16,7 @@ | |
| _SELECT_ALL_COLUMNS = selectors.all() | ||
|
|
||
|
|
||
| class ApplyToEachCol(BaseEstimator, TransformerMixin): | ||
| class ApplyToEachCol(SkrubBaseTransformer, TransformerMixin): | ||
|
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. while we're at it let's put the mixin before the base class |
||
| """ | ||
| Map a transformer to columns in a dataframe. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| from sklearn.base import BaseEstimator | ||
|
|
||
|
|
||
| class SkrubBaseTransformer(BaseEstimator): | ||
| """Base class for all skrub transformers. | ||
|
|
||
| This is a class that all skrub transformers inherit from. | ||
| For the moment, it's only used for the documentation url, but eventually | ||
| it will be used for other things as well. | ||
| """ | ||
|
rcap107 marked this conversation as resolved.
Outdated
|
||
|
|
||
| _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 | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1460,3 +1460,13 @@ def load_data(): | |||||||||
| pred = X.skb.apply(DummyClassifier(), y=y) | ||||||||||
| search = pred.skb.make_grid_search(scoring="roc_auc").fit({}) | ||||||||||
| assert search.results_.shape[0] == 1 | ||||||||||
|
|
||||||||||
|
|
||||||||||
| def test_learner_docstring(): | ||||||||||
| data_op, data = get_data_op_and_data("simple") | ||||||||||
| split = data_op.skb.train_test_split(data) | ||||||||||
| learner = data_op.skb.make_learner().fit(split["train"]) | ||||||||||
|
Comment on lines
+1466
to
+1468
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. do we need it to be fitted? otherwise maybe we can slightly simplify & speedup
Suggested change
Member
Author
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. whoops didn't see the comment, I'll add fix this when I'm cleaning up for the release |
||||||||||
| link = learner._get_doc_link() | ||||||||||
| assert link == ( | ||||||||||
| "https://skrub-data.org/stable/reference/generated/skrub.SkrubLearner.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.
@glemaitre could you give a bit of context on why this is needed here?