DOC document and deprecate missing attributes in MiniBatchKMeans #17864
Conversation
|
Could you add an entry in what's new to mention that we deprecate these attributes (this is more interesting than the documentation actually). Otherwise LGTM |
|
There was also |
| IGNORED = {'BayesianRidge', 'Birch', 'CCA', | ||
| 'LarsCV', 'Lasso', 'LassoLarsCV', 'LassoLarsIC', | ||
| 'OrthogonalMatchingPursuit', | ||
| 'PLSCanonical', 'PLSSVD', | ||
| 'PassiveAggressiveClassifier'} | ||
| 'PLSCanonical', 'PLSSVD'} |
There was a problem hiding this comment.
I found that 3 other estimators pass the test. We forgot to remove them from the blacklist in the dedicated doc PRs
|
Thanks! Generally looks good, there are some test failures however. |
| @@ -1831,7 +1857,7 @@ def partial_fit(self, X, y=None, sample_weight=None): | ||
| order='C', accept_large_sparse=False, | ||
| reset=is_first_call_to_partial_fit) | ||
|
|
||
| self.random_state_ = getattr(self, "random_state_", | ||
| self._random_state = getattr(self, "_random_state", |
There was a problem hiding this comment.
Do we also need to deprecate this one, or was it a typo / random_state_ never existed?
There was a problem hiding this comment.
I wasn't sure what to do about this one. In the PR descr I asked
random_state_ is only created and used in partial_fit. I think it's safe to directly privatize it. What do you think ?
There was a problem hiding this comment.
missed that, sorry. If it's only a matter of having a @Property it wouldn't hurt to deprecate (no need to document it though). If it's a pain, I agree there's little risk in just ignoring this. Up to you, feel free to self merge
There was a problem hiding this comment.
Let's deprecate it as well
Part of #14312
counts_andinit_size_are not documented as MiniBatchKMeans attributes.They are not useful for the user so I propose to deprecate them.
random_state_is only created and used inpartial_fit. I think it's safe to directly privatize it. What do you think ?Extracted from #17622 to facilitate the reviews.
ping @glemaitre