TST test_fit_docstring_attributes include properties #20190
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thomasjpfan
reviewed
Jun 5, 2021
Comment on lines
241
to
242
| if Estimator.__name__ in HAS_DEPRECATED_ATTRS: | ||
| pytest.skip("Estimator has undocumented deprecated attributes") |
There was a problem hiding this comment.
Turning off the whole test for an estimator because it has deprecated properties means we will not catch future attribute docstring errors. In the property finding code can we filter out deprecated properties? For example:
def _get_all_fitted_attributes(estimator):
"Get all the fitted attributes of an estimator including properties"
# attributes
fit_attr = list(estimator.__dict__.keys())
# properties
with warnings.catch_warnings():
warnings.filterwarnings("error", category=FutureWarning)
for name in dir(estimator.__class__):
obj = getattr(estimator.__class__, name)
if not isinstance(obj, property):
continue
# ignore properties that raises an AttributeError and deprecated
# properties
try:
getattr(estimator, name)
except (AttributeError, FutureWarning):
continue
fit_attr.append(name)
return [k for k in fit_attr if k.endswith('_') and not k.startswith('_')]There was a problem hiding this comment.
Your solution is better, thanks. And it actually revealed that I excluded an estimator by mistake, the property was not deprecated.
ogrisel
approved these changes
Jun 7, 2021
There was a problem hiding this comment.
I would be in favor of trying to implement what @thomasjpfan suggests in https://github.com/scikit-learn/scikit-learn/pull/20190/files#r646042494 but otherwise, LGTM.
thomasjpfan
added a commit
to thomasjpfan/scikit-learn
that referenced
this pull request
Jun 8, 2021
* TST enable test docstring params for feature extraction module (scikit-learn#20188) * DOC fix a reference in sklearn.ensemble.GradientBoostingRegressor (scikit-learn#20198) * FIX mcc zero divsion (scikit-learn#19977) * TST Add TransformedTargetRegressor to test_meta_estimators_delegate_data_validation (scikit-learn#20175) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> * TST enable n_feature_in_ test for feature_extraction module * FIX Uses points instead of pixels in plot_tree (scikit-learn#20023) * MNT n_features_in through the multiclass module (scikit-learn#20193) * CI Removes python 3.6 builds from wheel building (scikit-learn#20184) * FIX Fix typo in error message in `fetch_openml` (scikit-learn#20201) * FIX Fix error when using Calibrated with Voting (scikit-learn#20087) * FIX Fix RandomForestRegressor doesn't accept max_samples=1.0 (scikit-learn#20159) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * ENH Adds Poisson criterion in RandomForestRegressor (scikit-learn#19836) Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com> Co-authored-by: Alihan Zihna <alihanz@gmail.com> Co-authored-by: Alihan Zihna <a.zihna@ckhgbdp.onmicrosoft.com> Co-authored-by: Chiara Marmo <cmarmo@users.noreply.github.com> Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com> Co-authored-by: naozin555 <37050583+naozin555@users.noreply.github.com> Co-authored-by: Venkatachalam N <venky.yuvy@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * TST Replace assert_warns from decomposition/tests (scikit-learn#20214) * TST check n_features_in_ in pipeline module (scikit-learn#20192) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com> Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com> * Allow `n_knots=None` if knots are explicitly specified in `SplineTransformer` (scikit-learn#20191) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> * FIX make check_complex_data deterministic (scikit-learn#20221) * TST test_fit_docstring_attributes include properties (scikit-learn#20190) * FIX Uses the color max for colormap in ConfusionMatrixDisplay (scikit-learn#19784) * STY Changing .format method to f-string formatting (scikit-learn#20215) * CI Adds permissions for label action Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com> Co-authored-by: tsuga <2888173+tsuga@users.noreply.github.com> Co-authored-by: Conner Shen <connershen98@hotmail.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: mlondschien <61679398+mlondschien@users.noreply.github.com> Co-authored-by: Clément Fauchereau <clement.fauchereau@ensta-bretagne.org> Co-authored-by: murata-yu <67666318+murata-yu@users.noreply.github.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Brian Sun <52805678+bsun94@users.noreply.github.com> Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com> Co-authored-by: Alihan Zihna <alihanz@gmail.com> Co-authored-by: Alihan Zihna <a.zihna@ckhgbdp.onmicrosoft.com> Co-authored-by: Chiara Marmo <cmarmo@users.noreply.github.com> Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com> Co-authored-by: naozin555 <37050583+naozin555@users.noreply.github.com> Co-authored-by: Venkatachalam N <venky.yuvy@gmail.com> Co-authored-by: Nanshan Li <nanshanli@dsaid.gov.sg> Co-authored-by: solosilence <abhishekkr23rs@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The fitted attributes defined as properties are not properly tested in
test_fit_docstring_attributes.If an attribute is document, the check that the attribute exists works as expected. The opposite doesn't work, i.e. a property is not checked to be documented.
Many of the properties are attributes being deprecated, so we don't care if they are not documented as they will be removed soon. I put them in an exclude list.
A lot of them are
n_features_in_, which are ignored by this test for most modules, but they would not be found as undocumented in the current state of this test.a couple SearchCV estimators have an undocumented
classes_property that should have been catch by this test.