feat: Turn report_metrics of ComparisonReport into Displays #1520
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MarieSacksick
changed the title
Contributor
Coverage Report for backend
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
Member
|
From speaking with @GaelVaroquaux, it looks like this plot should be more general meaning that it should be a pairwise plot (comparing 2 scores). Time vs score is probably a good default but we should make it possible to tweak via the parameters. |
Contributor
Coverage Report for backend
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
MarieSacksick
force-pushed
the
timing_plot
branch
from
7b0ae03 to
c8c5ddb
Compare
Contributor
Coverage Report for |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File | Stmts | Miss | Cover | Missing |
|---|---|---|---|---|
| venv/lib/python3.12/site-packages/skore | ||||
| __init__.py | 23 | 0 | 100% | |
| _config.py | 28 | 0 | 100% | |
| exceptions.py | 4 | 4 | 0% | 4, 15, 19, 23 |
| venv/lib/python3.12/site-packages/skore/project | ||||
| __init__.py | 2 | 0 | 100% | |
| metadata.py | 67 | 0 | 100% | |
| project.py | 43 | 0 | 100% | |
| reports.py | 11 | 0 | 100% | |
| widget.py | 138 | 5 | 96% | 375–377, 447–448 |
| venv/lib/python3.12/site-packages/skore/sklearn | ||||
| __init__.py | 6 | 0 | 100% | |
| _base.py | 169 | 14 | 91% | 45, 58, 126, 129, 182, 185–186, 188–191, 224, 227–228 |
| find_ml_task.py | 61 | 0 | 100% | |
| types.py | 26 | 1 | 96% | 26 |
| utils.py | 1 | 0 | 100% | |
| venv/lib/python3.12/site-packages/skore/sklearn/_comparison | ||||
| __init__.py | 5 | 0 | 100% | |
| metrics_accessor.py | 203 | 3 | 98% | 170, 334, 1288 |
| report.py | 98 | 0 | 100% | |
| utils.py | 55 | 0 | 100% | |
| venv/lib/python3.12/site-packages/skore/sklearn/_cross_validation | ||||
| __init__.py | 5 | 0 | 100% | |
| metrics_accessor.py | 211 | 1 | 99% | 334 |
| report.py | 125 | 1 | 99% | 480 |
| venv/lib/python3.12/site-packages/skore/sklearn/_estimator | ||||
| __init__.py | 7 | 0 | 100% | |
| feature_importance_accessor.py | 143 | 2 | 98% | 216–217 |
| metrics_accessor.py | 382 | 9 | 97% | 162, 191, 193, 200, 291, 360, 364, 379, 414 |
| report.py | 166 | 2 | 98% | 454–455 |
| venv/lib/python3.12/site-packages/skore/sklearn/_plot | ||||
| __init__.py | 2 | 0 | 100% | |
| base.py | 5 | 0 | 100% | |
| style.py | 28 | 0 | 100% | |
| utils.py | 118 | 5 | 95% | 50, 74–76, 80 |
| venv/lib/python3.12/site-packages/skore/sklearn/_plot/metrics | ||||
| __init__.py | 6 | 0 | 100% | |
| confusion_matrix.py | 69 | 4 | 94% | 90, 98, 120, 228 |
| pair_plot.py | 67 | 8 | 88% | 54, 57, 89, 150, 164, 174, 178, 219 |
| precision_recall_curve.py | 230 | 1 | 99% | 716 |
| prediction_error.py | 160 | 0 | 100% | |
| roc_curve.py | 242 | 4 | 98% | 380, 497, 598, 791 |
| venv/lib/python3.12/site-packages/skore/sklearn/train_test_split | ||||
| __init__.py | 0 | 0 | 100% | |
| train_test_split.py | 49 | 0 | 100% | |
| venv/lib/python3.12/site-packages/skore/sklearn/train_test_split/warning | ||||
| __init__.py | 8 | 0 | 100% | |
| high_class_imbalance_too_few_examples_warning.py | 17 | 1 | 94% | 80 |
| high_class_imbalance_warning.py | 18 | 0 | 100% | |
| random_state_unset_warning.py | 10 | 0 | 100% | |
| shuffle_true_warning.py | 10 | 1 | 90% | 46 |
| stratify_is_set_warning.py | 10 | 0 | 100% | |
| time_based_column_warning.py | 21 | 1 | 95% | 73 |
| train_test_split_warning.py | 4 | 0 | 100% | |
| venv/lib/python3.12/site-packages/skore/utils | ||||
| __init__.py | 6 | 2 | 66% | 8, 13 |
| _accessor.py | 52 | 2 | 96% | 67, 108 |
| _environment.py | 27 | 0 | 100% | |
| _fixes.py | 8 | 0 | 100% | |
| _index.py | 5 | 0 | 100% | |
| _logger.py | 22 | 4 | 81% | 15–17, 19 |
| _measure_time.py | 10 | 0 | 100% | |
| _parallel.py | 38 | 3 | 92% | 23, 33, 124 |
| _patch.py | 13 | 5 | 61% | 21, 23–24, 35, 37 |
| _progress_bar.py | 45 | 0 | 100% | |
| _show_versions.py | 33 | 2 | 93% | 65–66 |
| _testing.py | 37 | 0 | 100% | |
| TOTAL | 3349 | 85 | 97% | |
| Tests | Skipped | Failures | Errors | Time |
|---|---|---|---|---|
| 830 | 5 💤 | 0 ❌ | 0 🔥 | 1m 1s ⏱️ |
Co-authored-by: Auguste Baum <[email protected]>
MarieSacksick
changed the title
Collaborator
|
[automated comment] Please update your PR with main, so that the |
glemaitre
reviewed
May 27, 2025
Member
glemaitre
left a comment
glemaitre
left a comment
There was a problem hiding this comment.
I think that we should think about the API question before to go further. It would have an impact on how the display will be created.
If we have a display with report_metrics, it only means that we will share a common display and the kind parameters will decide to show a representation or another.
| @@ -0,0 +1,220 @@ | |||
| import matplotlib.pyplot as plt | |||
|
|
|||
| from skore.sklearn._plot.base import Display | |||
Member
There was a problem hiding this comment.
Display is a Python protocol (https://peps.python.org/pep-0544/) and we don't need to inherit from it.
It only allows to specify the methods that an object should be implementing when calling isinstance(obj, Display).
Suggested change
| from skore.sklearn._plot.base import Display |
| self.ax_ = None | ||
| self.text_ = None | ||
|
|
||
| def plot(self, ax=None, **kwargs): |
Member
There was a problem hiding this comment.
It is this method that would benefit from the style.
Suggested change
| def plot(self, ax=None, **kwargs): | |
| @StyleDisplayMixin.style_plot | |
| def plot(self, ax=None, **kwargs): |
Member
There was a problem hiding this comment.
we don't need ax anymore. We decided with @auguste-probabl to reduce the API here.
Member
There was a problem hiding this comment.
You can also remove kwargs because it is unused.
Comment on lines
+105
to
+112
| @classmethod | ||
| def from_metrics( | ||
| cls, | ||
| metrics, | ||
| perf_metric_x, | ||
| perf_metric_y, | ||
| data_source=None, | ||
| ): |
Member
There was a problem hiding this comment.
The display should not expose this public function. The idea is that the reporters will be the only object that can create an instance of Display.
You can have a look at RocCurveDisplay (or PrecisionRecallDisplay). Basically I think we should keep the name _compute_data_for_display. However, we can adapt the input parameters.
| # - add kwargs (later) | ||
|
|
||
| return PairPlotDisplay.from_metrics( | ||
| metrics=self.metrics.report_metrics( |
Member
There was a problem hiding this comment.
One thing that I realized with the implementation now is that we are going to want most of the parameters to pass them to report_metrics.
Now, I'm thinking that it would means that the PairPlotDisplay is just a kind of plot associated with report_metrics. In short, I think that it would make sense to be able to write:
report.metrics.report_metrics().plot(kind="pair", x="fit_time", y="accuracy")but also
report.metrics.report_metrics().plot(kind="bar")And it allows to pass the arguments as:
report.metrics.report_metrics(data_source="train", ...).plot(kind="pair")
Comment on lines
+67
to
+69
| self.figure_ = None | ||
| self.ax_ = None | ||
| self.text_ = None |
Member
There was a problem hiding this comment.
For consistency, those are created only when plot is called. We can see in a subsequent PR if we want to make consistent this behaviour with an initialization.
Comment on lines
+135
to
+136
| x_label = _SCORE_OR_LOSS_INFO.get(perf_metric_x, {}).get("name", perf_metric_x) | ||
| y_label = _SCORE_OR_LOSS_INFO.get(perf_metric_y, {}).get("name", perf_metric_y) |
Member
There was a problem hiding this comment.
I think that those should be passed directly by the methods from the report. It would be handy because we would have access to the dictionary _SCORE_OR_LOSS_INFO in the report side.
Co-authored-by: Guillaume Lemaitre <[email protected]>
Co-authored-by: Guillaume Lemaitre <[email protected]>
Co-authored-by: Guillaume Lemaitre <[email protected]>
MarieSacksick
changed the title
Contributor
Author
|
Closing - fresh start in #1816. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As part of issue #1782, turn the report_metrics of ComparisonReport into a display. The plot of this display will be a pair plot to visualize one metric against another.
Add documentation.
This is part of the narrative "I have several models, how can I choose the best one?". The user will need:
blocked by #1788.