DOC: Add from_predictions example and other details to visualizations.rst
#30825
Conversation
There was a problem hiding this comment.
The first 2 plots (that you didn't touch) do no show in the rendered doc: https://output.circle-artifacts.com/output/job/5e0cade1-0c1e-4276-9a7c-728c32f8ff27/artifacts/0/doc/visualizations.html but the new plot you add does. Is this just a CI thing where it doesn't run if it hasn't been changed...?
Looks good otherwise, just some nits
I see 3 plots on the created docs. Could you clarify what you mean please? @lucyleeow |
|
Nevermind I see 3 plots now. I guess it was just my browser? sorry! |
|
No need to be sorry. |
|
Just a couple of comment more. @DeaMariaLeon could you resolve the comment from @lucyleeow that were addressed. |
doc/visualizations.rst
Outdated
| automatically resolve some ambiguities. For binary classification, the user must know | ||
| which column corresponds to the positive label (in this case, `y_pred`). |
There was a problem hiding this comment.
Not sure what your thoughts are on this @glemaitre but for decision_function, I thought we only returned one column? And we need to multiply by -1 if we wanted to switch pos_label?
I don't love the phrase "resolve some ambiguities", I would prefer something direct like, "must pass prediction values corresponding to the pos_label, for binary classification. For probabilities this means....For decision function values this means..."
There was a problem hiding this comment.
Yep, we can be precise here. It should be a single sentence. What about this formulation:
For :term:`predict_proba`, the column corresponding to the probability estimate of the `pos_label` class is selected while for :term:`decision_function`, the score is reverted (i.e. multiply by -1) when `pos_label` is not the label 1.
There was a problem hiding this comment.
@lucyleeow do you agree with the last sentence from Guillaume?
Reference Issues/PRs
Fixes #30767
What does this implement/fix? Explain your changes.
I tried to address all the points in the issue, and clarify a bit more.
Any other comments?
I moved ln 37, 38: "Be aware that we could get
the predictions from the support vector machine and then use
from_predictionsinstead of
from_estimator." To the bottom, above the new example (written a bit differently).I can leave it untouched if you prefer of course.
WDYT? @lucyleeow @glemaitre