-
-
Notifications
You must be signed in to change notification settings - Fork 25.2k
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
[MRG] Changed name n_components to n_connected_components in AgglomerativeClustering base class #13427
[MRG] Changed name n_components to n_connected_components in AgglomerativeClustering base class #13427
Conversation
To avoid breaking backwards compatibility we need to support users accessing n_components_ but provide a deprecation warning.. The easiest way to do this is to define n_components_ as a property which retrieves n_connected_components_ while raising a warning. I've not yet looked at your test failures. |
Thanks for your feedback and guidance Joel - I will try and incorporate that idea by adding commits to this PR |
Hi Joel - I just added the deprecation warning. Let me know if you agree on:
|
Concerning the test failures, I believe they are not related to this issue however |
If nothing is passed, it should behave the same as version 0.20. we will
say deprecated in 0.21 to be removed in 0.23
|
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.
The deprecation should be on the attribute retrieval, I.e. in the class:
@property
def n_components_(self):
Warn...
return self.n_connected_components_
Hi @jnothman - it seems the patch code coverage has decreased, do you know if I should update any of the tests ? Or are the tests not related to this change ? |
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.
You can get code coverage if you add a test that does something like:
with pytest.warns(DeprecationWarning):
n = my_agglomerative_clusterer.n_components_
assert n == my_agglomerative_clusterer.n_connected_components_
…ibute from AgglomerativeClustering class
Apologies @NicolasHug, I am having some issues with resolving a merge conflict I will look at this tomorrow |
5d58054
to
5d30766
Compare
@NicolasHug just added the whatsnew in commit #31354c3. Thanks to you @NicolasHug and @jnothman for your feedback and assistance ! I learned like crazy working on this issue. |
57b1d0d
to
4cbe2c1
Compare
0d44e5b
to
7642f64
Compare
…ion and AgglomerativeClustering class dosctrings
Apologies @NicolasHug @jnothman - after fixing indentation changes in commit #e48f5cc you requested I get a test failure on
|
line 471 in hierarchical should be And yes when in doubt, merge master, it never hurts. |
e48f5cc
to
6c76d7c
Compare
Thanks alot @NicolasHug, my mistake |
Good work @scouvreur.. Glad you've learnt a lot on the way! |
thx @scouvreur |
…ativeClustering base class (scikit-learn#13427) * Changed name n_components to n_connected_components in base class * Fixed line which exceeded PEP8 max of 79 chars * Fixed line 818 which exceeded PEP8 max of 79 chars * Added try and except to provide deprecation warning if passed * Updated deprecation and removal version numbers * Added deprecation of n_components using @Property generator * Makes FeatureAgglomeration class inherit n_connected_components_ attribute from AgglomerativeClustering class * Added test for DeprecationWarning when trying to access n_components * Removed @Property generator causing linting error * Fixed typo in test * Fixed flake8 error due to single line between 2 functions * Test fix attempt * Edited test function docstring * Corrected n_components deprecation test docstring * Fixed line continuation issue in AgglomerativeClustering base class * Added deprecation message as part of the @deprecated decorator * Added attribute deprecation information in the Attributes section of the AgglomerativeClustering base class docstring * Added test for deprecation warning message * Added attribute deprecation information in the Attributes section of the FeatureAgglomeration base class docstring * Fixed test issue and added longer match string * Edited n_components_ deprecation message to add double backticks * Fixed match string to reflect deprecation message change in test * Added name to list of contributors * Documented information in v0.21 changelog * Added cluster parent folder to documentation in v0.21 changelog * Removed myself from list of core contributors * Moved |API| subsection to the end of the list, and changed reference to Github username * Removed n_components deprecation documentation from FeatureAgglomeration and AgglomerativeClustering class dosctrings * Fix indentation on _fix_connectivity function call
…ativeClustering base class (scikit-learn#13427) * Changed name n_components to n_connected_components in base class * Fixed line which exceeded PEP8 max of 79 chars * Fixed line 818 which exceeded PEP8 max of 79 chars * Added try and except to provide deprecation warning if passed * Updated deprecation and removal version numbers * Added deprecation of n_components using @Property generator * Makes FeatureAgglomeration class inherit n_connected_components_ attribute from AgglomerativeClustering class * Added test for DeprecationWarning when trying to access n_components * Removed @Property generator causing linting error * Fixed typo in test * Fixed flake8 error due to single line between 2 functions * Test fix attempt * Edited test function docstring * Corrected n_components deprecation test docstring * Fixed line continuation issue in AgglomerativeClustering base class * Added deprecation message as part of the @deprecated decorator * Added attribute deprecation information in the Attributes section of the AgglomerativeClustering base class docstring * Added test for deprecation warning message * Added attribute deprecation information in the Attributes section of the FeatureAgglomeration base class docstring * Fixed test issue and added longer match string * Edited n_components_ deprecation message to add double backticks * Fixed match string to reflect deprecation message change in test * Added name to list of contributors * Documented information in v0.21 changelog * Added cluster parent folder to documentation in v0.21 changelog * Removed myself from list of core contributors * Moved |API| subsection to the end of the list, and changed reference to Github username * Removed n_components deprecation documentation from FeatureAgglomeration and AgglomerativeClustering class dosctrings * Fix indentation on _fix_connectivity function call
…AgglomerativeClustering base class (scikit-learn#13427)" This reverts commit 2d94859.
…AgglomerativeClustering base class (scikit-learn#13427)" This reverts commit 2d94859.
…ativeClustering base class (scikit-learn#13427) * Changed name n_components to n_connected_components in base class * Fixed line which exceeded PEP8 max of 79 chars * Fixed line 818 which exceeded PEP8 max of 79 chars * Added try and except to provide deprecation warning if passed * Updated deprecation and removal version numbers * Added deprecation of n_components using @Property generator * Makes FeatureAgglomeration class inherit n_connected_components_ attribute from AgglomerativeClustering class * Added test for DeprecationWarning when trying to access n_components * Removed @Property generator causing linting error * Fixed typo in test * Fixed flake8 error due to single line between 2 functions * Test fix attempt * Edited test function docstring * Corrected n_components deprecation test docstring * Fixed line continuation issue in AgglomerativeClustering base class * Added deprecation message as part of the @deprecated decorator * Added attribute deprecation information in the Attributes section of the AgglomerativeClustering base class docstring * Added test for deprecation warning message * Added attribute deprecation information in the Attributes section of the FeatureAgglomeration base class docstring * Fixed test issue and added longer match string * Edited n_components_ deprecation message to add double backticks * Fixed match string to reflect deprecation message change in test * Added name to list of contributors * Documented information in v0.21 changelog * Added cluster parent folder to documentation in v0.21 changelog * Removed myself from list of core contributors * Moved |API| subsection to the end of the list, and changed reference to Github username * Removed n_components deprecation documentation from FeatureAgglomeration and AgglomerativeClustering class dosctrings * Fix indentation on _fix_connectivity function call
Reference Issues/PRs
This PR is a fix for issue #13357
What does this implement/fix? Explain your changes.
As explained in issue #13357, this fix renames
n_components
ton_connected_components
asn_components
refers to something different in the rest of the codebase, but here refers to the attribute of connected components in the graph.Checked in the following scripts in the codebase for breaking changes:
and found none.