[MRG] Improve multi-metric scorer speed #10979
Conversation
65bbb14 to
9459bf9
Compare
|
ping @jnothman ? |
There was a problem hiding this comment.
I'd tweak this a bit. Firstly let's rename score_predict to score_predictions or score_precomputed. Secondly, let's add a staticmethod to these scorers called precompute_predictions.
In _multimetric_score we can then do something like:
precomputed = {}
for name, scorer in scorers.items():
if hasattr(scorer, 'score_precomputed'):
func = scorer.precompute_predictions
if func not in precomputed:
precomputed[func] = func(X_test)
score = scorer.score_precomputed(precomputed[func], y_test)
else:
score = scorer(X_test, y_test)
...Do you think this would be much harder to understand? It still can call prediction functions more times than memoising.
| def _is_predict(x): | ||
| return isinstance(x, _PredictScorer) | ||
|
|
||
| # We want to keep the memmap and score types in a single |
There was a problem hiding this comment.
I don't know what you mean by the memmap here.
There was a problem hiding this comment.
In the loop at https://github.com/scikit-learn/scikit-learn/pull/10979/files#diff-60033c11a662f460e1567effd5faa6f0R625
The scores are checked for being scalars and are unwrapped if they are memmaped.
tmp_scores contains the scores before being processed this way (it avoids doing the checks for each type of scorers)
There was a problem hiding this comment.
Ah, I now see what you mean. I think this makes sense to you as the one changing the code, but it would be hard for the reader to follow what you're referring to in this comment. It's better without the comment.
|
And thanks for the ping |
|
@gamebusterz |
I agree with this statement of @jnothman. Changing the name will be more explicit. |
Previously multi metric scoring called the `predict` method of an estimator once for each scorer, this could lead to drastic increases in costs. This change avoids calling the scorers directly and instead allows to call the scorer with the predicted results. This is only done for `_PredictScorer` and `_ProbaScorer` generated with `make_scorer`, this means that `_ThresholdScorer` and scorers not generated with `make_scorer` do not benefit from this change. Works on improving scikit-learn#10802
|
@glemaitre Just did the changes (I forgot to push them last week...). Regarding the static method, I am not convinced this is the best way to go. In order to be constructive, I propose to attack the caching right away, and volunteer to do it (it may however be a big piece so I expect it to take a while). |
|
I don't think we generally want caching in the estimators themselves...
|
|
@jnothman Hmm, I may have misunderstood you in the issue then ^^ If it's not too much of a hassle, why exactly wouldn't we want caching in the estimators (I'm not too familiar with the implementation of sklearn so I may miss something obvious) ? |
|
Because the dataset being predicted might be large, and we don't want to force the storage of that data. |
|
@jnothman When you say it like that, there is indeed an obvious reason to avoid storage by default :) Should I try the method proposed (above)[https://github.com//pull/10979#pullrequestreview-114281530] or is this good to go now that the changes in the names are applied ? |
|
@gamazeps Any news |
|
Wooopsy... I git caught up with administrative issues and did not have much time to dedicate to open source in the last weeks, I expect to have more time in two weeks. Does that work for you ? Cheers |
|
@gamazeps are you able to work on this, or should we find someone else to complete it? (Another alternative we may be considering would allow a single scorer function to compute multiple results, returning a dict.) |
Along these lines, WDYT about introducing |
|
Along these lines, WDYT about introducing make_multi_scorer and a MultiScorer class?
Not sure how this specifically helps. I'd like to understand better
the API that cross validation would expect and how it would produce
results, and then work out how to help the user express what they
want... but the names don't tell me much.
|
|
#14484 is the current candidate, @marctorrellas. Review from a user perspective is very welcome. |
|
maybe this can be closed then? |
|
We usually don't close PR unless an equivalent implementation has been merged elsewhere. When/if #14484 gets merged, we will close this one |
|
fixed in #14593 |
Reference Issues/PRs
Works on improving #10802
What does this implement/fix? Explain your changes.
Previously multi metric scoring called the
predictmethod of anestimator once for each scorer, this could lead to drastic increases in
costs.
This change avoids calling the scorers directly and instead allows to
call the scorer with the predicted results.
This is only done for
_PredictScorerand_ProbaScorergenerated withmake_scorer, this means that_ThresholdScorerand scorers notgenerated with
make_scorerdo not benefit from this changeAny other comments?
This implements a solution proposed by @jimmywan and the demarch seemed ok to @jnothman another solution would have been to force the estimator to cache its
predict,predict_probaanddecision_functionmethods.