[WIP] Estimator Tags #10264
[WIP] Estimator Tags #10264
Conversation
…tfidfTransformer)
…later add more checks for nd-data (as in isotonic) if we like to.
…as init parameter
…output decision function
… (BACKWARD INCOMPATIBLE)
# Conflicts: # sklearn/feature_extraction/image.py # sklearn/tests/test_common.py # sklearn/utils/estimator_checks.py
# Conflicts: # sklearn/tests/test_common.py # sklearn/utils/estimator_checks.py
…ut the merge is complete 🙌🏽
|
Many of the tests are failing after merging new changes after 5 months. Many of these failures are due to checks being run on estimators that they shouldn't be. As an example, |
|
This pull request introduces 3 alerts - view on lgtm.com new alerts:
Comment posted by lgtm.com |
|
maybe instead of running the whole suite try running |
|
Prof. @amueller does it make sense that I'm getting 9 failures on the current master branch (not this one)? I'm beginning to think that the issue lies with the testing framework change, as you mentioned earlier |
| @@ -20,3 +20,5282 @@ Previous Releases | |||
| Version 0.14 <whats_new/v0.14.rst> | |||
There was a problem hiding this comment.
This file was not correctly merged. Make sure this file is not touched in the diff, only doc/whats_new/v0.20.rst
| parameters to ``__init__`` in the ``_required_parameters`` class attribute, | ||
| which is a list or tuple. If ``_required_parameters`` is only ``["estimator"]`` or ``["base_estimator"]``, then the | ||
| estimator will be instantiated with an instance of | ||
| ``LinearDiscriminantAnalysis`` (or ``RidgeRegression`` if the estimator is a regressor) in the tests. |
There was a problem hiding this comment.
it seems strange to assume that if the metaestimator is a regressor, the estimator passed into it is.
There was a problem hiding this comment.
What you're saying is that this should be another tag for the meta-estimator? "requires-base-estimator being "regressor", "classifier", "transformer", "any"?
|
|
||
| In addition to the tags, estimators are also need to declare any non-optional | ||
| parameters to ``__init__`` in the ``_required_parameters`` class attribute, | ||
| which is a list or tuple. If ``_required_parameters`` is only ``["estimator"]`` or ``["base_estimator"]``, then the |
There was a problem hiding this comment.
Surely this can be almost always inferred from signature. What does having it listed add?
There was a problem hiding this comment.
being explicit about it. This way we can detect if a parameter was left without a default by accident. Right now we require all estimators to be default-constructible. This PR will allow testing non-default-constructible estimators. But we still want to enforce that most estimators are default-constructible.
| # raises error on malformed input for transform | ||
| if hasattr(X, 'T') and not _safe_tags(transformer, "stateless"): | ||
| # If it's not an array, it does not have a 'T' property | ||
| assert_raises(ValueError, transformer.transform, X.T) |
There was a problem hiding this comment.
This is not the assertion that's failing. The one at the end of this function is the one that's failing on stateless transformers, though it is a repeat of this with more detail.
|
closing as I continued in the original PR |
Reference Issues/PRs
Original PR: #8022
Fixes #6599, #7737, #6554, #6715
Also see #6510
What does this implement/fix? Explain your changes.
Taken over from @amueller. Implements descriptive tags for estimators e.g.
'stateless': False,'deterministic': TrueAny other comments?
See #8022 for more details