If we go with this implementation, I thought this change could be used for other multi cv displays. Not 100% sure on this change though.

I was thinking that I could add a separate test that uses names (instead of estimator_name), so we can delete this test once we've switched all these estimators across to names?
This avoids complicate if/else inside this test.
WDYT?

Yes I agree.

I thought about adding from_cv_results to this (test_plot_roc_curve_pos_label) test BUT I see we create an imbalanced training set, and pass a separate test set to from_predictions and from_estimator, which we could not do with from_cv_results, so it feels like it would miss the spirit of this test. I think we should add a separate test for pos_label from_cv_results, WDYT?

In general, I don't mind to have separated test for from_cv_results because we do much more stuff inside and the test might end-up with an if/else. So instead let's define a dedicated test.

@glemaitre I don't quite follow this comment?

So here, we have string and the positive class is "cancer" that corresponds to classes_[0].
So we are in a non-default scenario (classes_[1] is usually the positive class) and thus we can verify that everything works as expected: get_response_value returns a probability of 1 - p(classes_[1]) (or return the probability of classes_[0]) or that negate the decision function.

But what does "are betrayed by the class imbalance" mean? How does class imbalance affect pos_label ?

Does fpr also take a list of ndarray? I think that it gets converted to a list on _deprecate_singular. As a "user" I would be a bit confused there I think.

That was a copy pasta! Fixed now.

These are deprecated now - they'll have the red deprecated box (via the .. deprecated), which I am hoping will be enough.

If the user does use it, they will get a warning and what they input will be put into a list and passed. This is all stated in the warning. I thought about adding it to the docstring here as well, but thought it wasn't necessary and would make the parameter descriptions un-necessarily long and complicated. Open to other opinions though!

I am wondering it this is still a good default, to name things "Classifier". Not sure if it provides more info, the legend label could just have the AUC value (or no legend)

Ideally we do parameter checking first, but I think this is okay...?

Since we only need those in plot, we can avoid the _ and delay the conversion in plot itself.

This adds reference the user guide section on roc metrics and visualization classes.

I initially wanted to follow the 'Read more in our user guide' line that we use everywhere else, but since we are reference 2 places in the user guide, i thought it would be okay to change for this case but I am not 100%. Happy to amend.

Thinking again, it is a breaking change. So better to deprecate it. However, we don't have the same constraint as for the estimator so we can do it in __init__. I think that we can create a small function because the same deprecation might happen in plot.

I have forgotten where we were up to in our discussions with this. Are we happy with the term 'name' ? I think it is reasonable.

And yes it is a breaking change so we should deprecate estimator_name and add name right? And I can add a function that errors if both params are given and warns if the deprecated one is.

Let's be gentle and deprecate it as well.

In the long-term, I'm thinking that we could come with a new API way to specify those. For instance:

display = Display.from_estimator().set_style(chance_level_kw={"color": "tab:blue"})
display.plot()
display.plot(chance_level_kw={"color": "tab:blue"})  # overwrite the one set by `set_style`

I could be handy because you would not need to pass each tim the style. But let's discuss in another PR.

Sorry I do not follow this.

In the example above, do you mean fold_line_kwargs instead of chance_level_kw ?

If you mean chance_level_kw, there is only one per plot, so you only need to pass this once?

If you mean fold_line_kwargs, you can just pass one set of kwargs and fold_line_kwargs will apply it to all curves. Unless you mean we want to pass some default kwargs that apply to all curves, and then also some specific ones (e.g., colour) that should be applied to each curve?

I think it is fine to have it here because we will need it between different displays.

I would do that in the _validate_plot_params

I think this is easier but will require a 'not so pretty':

if self.__class__.__name__ in (<classes requiring name_>):
    self.name_ = _convert_to_list_leaving_none()

potentially, I would think that we need to do that in the _validate_plot_params where in each class, we could modify it e.g.:

def _validate_plot_params(self, ax=ax, name=name, fpr=self.fpr, ...):
    ax, fig, name = super()._validate_plot_params(ax=ax, name=name)
    fpr = ...
    return ax, fig, name, fpr, ...

The problem is that _BinaryClassifierCurveDisplayMixin is used for many display classes not just ROC curve.

We could have a **kwarg at the end, where all these parameters are validated with _convert_to_list_leaving_none BUT we have to set a e.g., self.tpr_ and I don't know how you would do this...?

I would also include this check in the _validate_plot_params

I would say that there is a bug here (but we don't have the test yet). We should split the sample_weight to get the testing portion as well. So the above _validation_from_predictions_param` is indeed useful :)

_validate_from_predictions_params only checks that sample_weight is the same length as y_true and y_pred,
But you are right, this is a bug and I should be using _safe_indexing

I think the problem is that _validation_from_predictions_param and _validate_and_get_response_values are very specific for the from_estimator and from_prediction methods.

Maybe it is a good idea to add a _validate_cv_result_params method?

Yes I agree.

In general, I don't mind to have separated test for from_cv_results because we do much more stuff inside and the test might end-up with an if/else. So instead let's define a dedicated test.

So here, we have string and the positive class is "cancer" that corresponds to classes_[0].
So we are in a non-default scenario (classes_[1] is usually the positive class) and thus we can verify that everything works as expected: get_response_value returns a probability of 1 - p(classes_[1]) (or return the probability of classes_[0]) or that negate the decision function.