Skip to content

Conversation

@auguste-probabl
Copy link
Contributor

@auguste-probabl auguste-probabl commented May 7, 2025

Demo:

from sklearn.datasets import make_classification
from sklearn.linear_model import LogisticRegression
from skore import ComparisonReport, CrossValidationReport
X, y = make_classification(class_sep=0.1, random_state=42)
estimator_1 = LogisticRegression()
estimator_2 = LogisticRegression(C=2)  # Different regularization
report_1 = CrossValidationReport(estimator_1, X, y)
report_2 = CrossValidationReport(estimator_2, X, y)
report = ComparisonReport({"model1": report_1, "model2": report_2})
report.metrics.roc().plot()

2025-05-13T15_07_38_screenshot

from sklearn.datasets import make_classification
from sklearn.linear_model import LogisticRegression
from skore import ComparisonReport, CrossValidationReport
X, y = make_classification(
  class_sep=0.1, n_classes=3, n_clusters_per_class=1, random_state=42
)
estimator_1 = LogisticRegression()
estimator_2 = LogisticRegression(C=2)  # Different regularization
report_1 = CrossValidationReport(estimator_1, X, y)
report_2 = CrossValidationReport(estimator_2, X, y)
report = ComparisonReport({"model1": report_1, "model2": report_2})
display = report.metrics.roc()
display.plot()

2025-05-12T17_21_06_screenshot


Depends on #1668, #1682

TODO:

  • n_curves is not calculated correctly in the comparison-cv case (unclear in what way)
  • Binary classification: legend of chance level is not added for some reason (fixed in fix(roc-curve): Show chance level in the legend #1682)
  • Move new tests closer to existing tests
  • Deal with multiclass + pos_label is not None case: pos_label has no effect

@github-actions
Copy link
Contributor

github-actions bot commented May 12, 2025

Documentation preview @ 47cd9d4

@auguste-probabl
Copy link
Contributor Author

Note that the legend placement for the multiclass case depends on the size of the plot: here is what happens if the window is small:
2025-05-14T10_13_22_screenshot

@auguste-probabl auguste-probabl force-pushed the push-klxmqlurrwpq branch 2 times, most recently from e397976 to 82468ec Compare May 14, 2025 08:24
@auguste-probabl auguste-probabl marked this pull request as ready for review May 14, 2025 08:24
@github-actions
Copy link
Contributor

github-actions bot commented May 14, 2025

Coverage

Coverage Report for skore/
FileStmtsMissCoverMissing
venv/lib/python3.12/site-packages/skore
   __init__.py230100% 
   _config.py280100% 
   exceptions.py440%4, 15, 19, 23
venv/lib/python3.12/site-packages/skore/project
   __init__.py20100% 
   metadata.py670100% 
   project.py430100% 
   reports.py110100% 
   widget.py138596%375–377, 447–448
venv/lib/python3.12/site-packages/skore/sklearn
   __init__.py60100% 
   _base.py1691491%45, 58, 126, 129, 182, 185–186, 188–191, 224, 227–228
   find_ml_task.py610100% 
   types.py200100% 
venv/lib/python3.12/site-packages/skore/sklearn/_comparison
   __init__.py50100% 
   metrics_accessor.py205398%165, 329, 1283
   report.py950100% 
   utils.py580100% 
venv/lib/python3.12/site-packages/skore/sklearn/_cross_validation
   __init__.py50100% 
   metrics_accessor.py204199%321
   report.py1080100% 
venv/lib/python3.12/site-packages/skore/sklearn/_estimator
   __init__.py70100% 
   feature_importance_accessor.py143298%216–217
   metrics_accessor.py367897%181, 183, 190, 281, 350, 354, 369, 404
   report.py1550100% 
venv/lib/python3.12/site-packages/skore/sklearn/_plot
   __init__.py20100% 
   base.py50100% 
   style.py280100% 
   utils.py118595%50, 74–76, 80
venv/lib/python3.12/site-packages/skore/sklearn/_plot/metrics
   __init__.py50100% 
   confusion_matrix.py69494%90, 98, 120, 228
   precision_recall_curve.py174298%521, 524
   prediction_error.py1600100% 
   roc_curve.py242498%380, 497, 598, 791
venv/lib/python3.12/site-packages/skore/sklearn/train_test_split
   __init__.py00100% 
   train_test_split.py490100% 
venv/lib/python3.12/site-packages/skore/sklearn/train_test_split/warning
   __init__.py80100% 
   high_class_imbalance_too_few_examples_warning.py17194%80
   high_class_imbalance_warning.py180100% 
   random_state_unset_warning.py100100% 
   shuffle_true_warning.py10190%46
   stratify_is_set_warning.py100100% 
   time_based_column_warning.py21195%73
   train_test_split_warning.py40100% 
venv/lib/python3.12/site-packages/skore/utils
   __init__.py6266%8, 13
   _accessor.py52296%67, 108
   _environment.py270100% 
   _fixes.py80100% 
   _index.py50100% 
   _logger.py22481%15–17, 19
   _measure_time.py100100% 
   _parallel.py38392%23, 33, 124
   _patch.py13561%21, 23–24, 35, 37
   _progress_bar.py450100% 
   _show_versions.py33293%65–66
   _testing.py120100% 
TOTAL31457397% 

Tests Skipped Failures Errors Time
786 5 💤 0 ❌ 0 🔥 59.970s ⏱️

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a partial review. I still have to look in details the roc_curve file and the tests but I thought that it could already be useful to look at those.

@auguste-probabl auguste-probabl force-pushed the push-klxmqlurrwpq branch 2 times, most recently from 15d93fd to 47cd9d4 Compare May 23, 2025 09:15
auguste-probabl added a commit to auguste-probabl/skore that referenced this pull request May 23, 2025
Starting from probabl-ai#1669, it
is sometimes possible to draw plots onto (a `ndarray` of) several
Axes rather than just a single Axes. In fact, starting from
probabl-ai#1720, the same data might be
drawn on one or the other depending on user parameters.

In this context, allowing the user to pass `ax` might add too much
complexity compared to how useful it might be; it is considered an
advanced use-case which is difficult to motivate.

This PR thus removes the ability to pass user-created `ax`.
auguste-probabl added a commit to auguste-probabl/skore that referenced this pull request May 23, 2025
Starting from probabl-ai#1669, it
is sometimes possible to draw plots onto (a `ndarray` of) several
Axes rather than just a single Axes. In fact, starting from
probabl-ai#1720, the same data might be
drawn on one or the other depending on user parameters.

In this context, allowing the user to pass `ax` might add too much
complexity compared to how useful it might be; it is considered an
advanced use-case which is difficult to motivate.

This PR thus removes the ability to pass user-created `ax`.
auguste-probabl added a commit to auguste-probabl/skore that referenced this pull request May 23, 2025
Starting from probabl-ai#1669, it
is sometimes possible to draw plots onto (a `ndarray` of) several
Axes rather than just a single Axes. In fact, starting from
probabl-ai#1720, the same data might be
drawn on one or the other depending on user parameters.

In this context, allowing the user to pass `ax` might add too much
complexity compared to how useful it might be; it is considered an
advanced use-case which is difficult to motivate.

This PR thus removes the ability to pass user-created `ax`.
auguste-probabl added a commit to auguste-probabl/skore that referenced this pull request May 23, 2025
Starting from probabl-ai#1669, it
is sometimes possible to draw plots onto (a `ndarray` of) several
Axes rather than just a single Axes. In fact, starting from
probabl-ai#1720, the same data might be
drawn on one or the other depending on user parameters.

In this context, allowing the user to pass `ax` might add too much
complexity compared to how useful it might be; it is considered an
advanced use-case which is difficult to motivate.

This PR thus removes the ability to pass user-created `ax`.
@glemaitre glemaitre self-requested a review May 23, 2025 16:15
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I check in depth the roc_curve file. I have a couple of nit pickings.

Otherwise, I think that we can add a couple of tests for the kwargs (error and behaviour) and checking the dataframe as well.

I need to figure out some details for the legend.

@thomass-dev
Copy link
Collaborator

thomass-dev commented May 26, 2025

[automated comment] Please update your PR with main, so that the pytest workflow status will be reported.

@glemaitre glemaitre self-requested a review May 26, 2025 17:20
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We can go ahead and revisit the legend all at once.

@glemaitre glemaitre merged commit 8d08dac into probabl-ai:main May 26, 2025
24 checks passed
@glemaitre
Copy link
Member

Nice, two plots to go. @sylvaincom You could start to look at creating the .frame specifically for this display. This way we could start to check the API.

@auguste-probabl auguste-probabl deleted the push-klxmqlurrwpq branch June 4, 2025 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants