feat: Add ComparisonReport to compare instances of EstimatorReport

[thomass-dev]

• Rename to ComparisonReport
• Rebase on top of docs(future feat): Example driven development #1239 and adapt
• Raise if report.metrics.accuracy(data_source="train") is called with at least one EstimatorReport that does not have training data
• Test
• Docstrings
  • MetricsAccessor
• Move index column "#0" in front of each metric
•  Pass report names in comparator
• Update plots legend (deferred to future PR)
  • The actual RocCurveDisplay needs a full refactor to be splitted by use-case: estimator report, cross-validation report and finally comparison report. In each of these use-cases, there is two scenarios with binary classification and multi-class classification. Otherwise, it will be unmaintainable.
• Investigate missing metrics in report_metrics (deferred to future PR)
  • The logic is split between report_metrics and available_if; it should be merged (ideally everything in available_if?)
• Refactor to make CrossValidationReport depend on it (deferred to future PR)
• Change EstimatorReport repr? Issue EstimatorReport repr method has unintended side-effects #1293

Closes #1245

Co-authored-by: Auguste auguste@probabl.ai
Co-authored-by: Sylvain sylvain@probabl.ai

[github-actions]

Documentation preview @ cdef817

[glemaitre]

Could you elaborate on why it makes sense for the cross validation report to reuse the comparator. Is it about inheritance or about some internal routines?

[thomass-dev]

Could you elaborate on why it makes sense for the cross validation report to reuse the comparator. Is it about inheritance or about some internal routines?

The goal is to see if some code can be factorized between comparison and cross-validation reports, as there is a lot of duplication between their metric accessors.

[sylvaincom]

I believe the ComparisonReport does not appear in the API?

[auguste-probabl]

I believe the ComparisonReport does not appear in the API?

Oh sorry, we forgot! On it

[glemaitre]

Some thinking once I check the example

[glemaitre]

I see that we are going to potentially increase the number of examples pretty quick (it is already a huge issue in scikit-learn).

I think that it would be best to think of incorporating the usage of the comparator in existing example. I think that we could add a section at the following one: https://skore.probabl.ai/dev/auto_examples/use_cases/plot_employee_salaries.html

The idea would to train the linear & gbdt model and make the comparison using the comparator in the section.

Later when the comparator supports cross-validation report, then we can remove the sections and directly compare the results from the cross-validation.

[glemaitre]

Looking at the rendering and at a first glance I find the level_0 integral index weird. I need to think more about it.

[glemaitre]

I'll not merge until it is fixed ;P

[glemaitre]

I would say that there is not need to show that :)

[glemaitre]

For me this check should be delayed at computation of the metric. We are able to have a hash on the data or to know that they differ and thus warn properly.

Otherwise we will be raising too many potential true positive.

[thomass-dev]

@glemaitre i have rebased our work with your last changes on the plot API (commit ⬆️ 784c022 + ⬇️ b91cdd0). Can you check it?

This shows me once again that we should refactor the metrics_accessors.py files to reduce duplicate code.

[glemaitre]

Working on a review.

[glemaitre]

Here, is a partial review. I'll continue after the demo

[glemaitre]

I would create two separate fixture.

[glemaitre]

The error message tells me that it is not informative enough the end-user. So we should probably have if not isinstance(reports, Iterable) to mention that we want a list of reports.

[glemaitre]

Those two cases look redundant. Why not only use the latest one?

[glemaitre]

Actually, you should be able using the external data_source. I would expect something like this to work on the side of the end-user.

comparator.metrics.report_metrics(data_source="X_y", X=X, y=y)

[glemaitre]

Since you are accumulating in a set, I would differ raising the error after the loop because we can provide more information regarding the ML tasks of all estimators:

if len(ml_taks) > 1:
    raise ValueError(
        "Not all estimators were not trained on the same machine-learning task."
        "The tasks of the provided reports is: {ml_tasks}."
    )

I would probably store the tasks into a list to raise a better error message and have 1 report, 1 task. It helps in the debugging task.

[glemaitre]

Is it used?

[glemaitre]

I changed this in on of the PR, it should be called _SCORE_OR_LOSS_ICONS and be a dict.

[glemaitre]

Also we should be able to remove report_metrics and custom_metrics now

[glemaitre]

So here, we should be supporting the X_y case. When it will come to implement for the CrossValidationReport, this option is indeed not supported.

So it makes me think that we might want to store in the __init__ of the report, what type of reporter we get and we might raise an error in the different metrics based on that.

[glemaitre]

With the EstimatorReport, the aggregate is not useful. It is only an interesting option to compute a statistic on several splits.

[glemaitre]

Let's start simple and avoid to add the enumerate since the user can override the report_names.