[MRG] iforest chunks for score_samples #13283
Conversation
|
@ngoix please put the PR title to MRG when ready on your side |
tests fix flake8 fix ci
| @@ -325,3 +326,26 @@ def test_behaviour_param(): | |||
| clf2 = IsolationForest(behaviour='new', contamination='auto').fit(X_train) | |||
| assert_array_equal(clf1.decision_function([[2., 2.]]), | |||
| clf2.decision_function([[2., 2.]])) | |||
|
|
|||
|
|
|||
| # mock get_chunk_n_rows to actually test more than one chunk (here one | |||
There was a problem hiding this comment.
any idea how to merge these 2 tests ? they are the same, just with a different mocked chunk size
There was a problem hiding this comment.
Use pytest's monkeypatch fixture rather than unittest.mock alone?
But for this to be a reasonable test, you also need to test that the Mock has been called (or does patch do that?)
|
@agramfort this is ready for reviews |
sklearn/ensemble/iforest.py
Outdated
| " be removed in 0.22.", DeprecationWarning) | ||
| return self._threshold_ | ||
|
|
||
| def _compute_chunked_score_samples(self, X, working_memory=None): |
There was a problem hiding this comment.
If not for testing, why do we need the working_melody parameter here?
There was a problem hiding this comment.
Why do we need the working_memory parameter here?
There was a problem hiding this comment.
it is not useful indeed, thanks
| @@ -325,3 +326,26 @@ def test_behaviour_param(): | |||
| clf2 = IsolationForest(behaviour='new', contamination='auto').fit(X_train) | |||
| assert_array_equal(clf1.decision_function([[2., 2.]]), | |||
| clf2.decision_function([[2., 2.]])) | |||
|
|
|||
|
|
|||
| # mock get_chunk_n_rows to actually test more than one chunk (here one | |||
There was a problem hiding this comment.
Use pytest's monkeypatch fixture rather than unittest.mock alone?
But for this to be a reasonable test, you also need to test that the Mock has been called (or does patch do that?)
|
Tests failing |
sklearn/ensemble/iforest.py
Outdated
| " be removed in 0.22.", DeprecationWarning) | ||
| return self._threshold_ | ||
|
|
||
| def _compute_chunked_score_samples(self, X, working_memory=None): |
There was a problem hiding this comment.
Why do we need the working_memory parameter here?
doc/whats_new/v0.21.rst
Outdated
| @@ -144,6 +144,9 @@ Support for Python 3.4 and below has been officially dropped. | |||
| by avoiding keeping in memory each tree prediction. :issue:`13260` by | |||
| `Nicolas Goix`_. | |||
|
|
|||
| - |Efficiency| :class:`ensemble.IsolationForest` now uses chunks of data at | |||
| prediction step. :issue:`13283` by `Nicolas Goix`_. | |||
There was a problem hiding this comment.
Perhaps say something like "capping the memory usage"
|
THanks @ngoix! |
…utation (scikit-learn#13283)" This reverts commit bce9351.
…utation (scikit-learn#13283)" This reverts commit bce9351.
fixes #12040
is currently based on top of #13260 (so do not review yet)
needs to be rebased on #13251