Skip to content

TST raise explicit error when tags are missing#30248

Merged
glemaitre merged 3 commits intoscikit-learn:mainfrom
adrinjalali:tests/tags
Nov 12, 2024
Merged

TST raise explicit error when tags are missing#30248
glemaitre merged 3 commits intoscikit-learn:mainfrom
adrinjalali:tests/tags

Conversation

@adrinjalali
Copy link
Copy Markdown
Member

@adrinjalali adrinjalali commented Nov 8, 2024

Fixes #30237

This adds an explicit error when tags are missing which would result in failure of collecting tests. This gives an explicit error on why the collection fails.

@adrinjalali adrinjalali added the Developer API Third party developer API related label Nov 8, 2024
@adrinjalali adrinjalali added this to the 1.6 milestone Nov 8, 2024
@adrinjalali adrinjalali mentioned this pull request Nov 8, 2024
Closed
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 8, 2024 •
edited
Loading

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 8caca2f. Link to the linter CI: here

thomasjpfan
thomasjpfan reviewed Nov 12, 2024
View reviewed changes
sklearn/utils/estimator_checks.py


def _yield_api_checks(estimator):
if not isinstance(estimator, BaseEstimator):
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we made the decision to only really support estimators that inherit from BaseEstimator? If so, is it documented somewhere so third-party estimator developers can know.

Copy link
Copy Markdown
Member Author

@adrinjalali adrinjalali Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, documented:

scikit-learn/doc/developers/develop.rst

Lines 95 to 98 in 296aba8

Out of all the methods that an estimator implements, ``fit`` is usually the one you
want to implement yourself. Other methods such as ``set_params``, ``get_params``, etc.
are implemented in :class:`~sklearn.base.BaseEstimator`, which you should inherit from.
You might need to inherit from more mixins, which we will explain later.

And the example:

scikit-learn/doc/developers/develop.rst

Lines 313 to 344 in 296aba8

>>> import numpy as np
>>> from sklearn.base import BaseEstimator, ClassifierMixin
>>> from sklearn.utils.validation import validate_data, check_is_fitted
>>> from sklearn.utils.multiclass import unique_labels
>>> from sklearn.metrics import euclidean_distances
>>> class TemplateClassifier(ClassifierMixin, BaseEstimator):
...
... def __init__(self, demo_param='demo'):
... self.demo_param = demo_param
...
... def fit(self, X, y):
...
... # Check that X and y have correct shape, set n_features_in_, etc.
... X, y = validate_data(self, X, y)
... # Store the classes seen during fit
... self.classes_ = unique_labels(y)
...
... self.X_ = X
... self.y_ = y
... # Return the classifier
... return self
...
... def predict(self, X):
...
... # Check if fit has been called
... check_is_fitted(self)
...
... # Input validation
... X = validate_data(self, X, reset=False)
...
... closest = np.argmin(euclidean_distances(X, self.X_), axis=1)
... return self.y_[closest]

sklearn/utils/estimator_checks.py
yield sparse_format + "_64", X


@ignore_warnings(category=FutureWarning)
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For other reviewers, this is just shifting the original test down.

@glemaitre glemaitre merged commit 3ec4457 into scikit-learn:main Nov 12, 2024
@adrinjalali adrinjalali deleted the tests/tags branch November 12, 2024 16:24
jeremiedbb pushed a commit to glemaitre/scikit-learn that referenced this pull request Nov 14, 2024
@adrinjalali @jeremiedbb
TST raise explicit error when tags are missing (scikit-learn#30248)
a757e81
jeremiedbb pushed a commit that referenced this pull request Nov 14, 2024
@adrinjalali @jeremiedbb
TST raise explicit error when tags are missing (#30248)
6f424d0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@glemaitre glemaitre glemaitre approved these changes

Assignees

No one assigned

Labels

Developer API Third party developer API related module:utils No Changelog Needed

Projects

Developer API and third party library support
Status: Done

Milestone

1.6

Development

Successfully merging this pull request may close these issues.

BUG: Test collection for Transformer fails

3 participants

@adrinjalali @thomasjpfan @glemaitre