Skip to content

Conversation

@waridrox
Copy link
Contributor

Following up on #1757

CC: @glemaitre

@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.

@waridrox waridrox force-pushed the refactorConfusionMatrix branch from 9e30d04 to 62080ea Compare May 26, 2025 10:37
@waridrox waridrox marked this pull request as draft May 26, 2025 14:21
@waridrox waridrox force-pushed the refactorConfusionMatrix branch from 62080ea to e59f761 Compare May 26, 2025 16:54
@auguste-probabl auguste-probabl linked an issue Jun 5, 2025 that may be closed by this pull request
@glemaitre glemaitre self-requested a review June 10, 2025 06:07
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.

In terms of API, we should remove sample_weight (we are not currently supporting it).

For the parameter display_labels, include_values, normalize and value_format, they should be only used in .plot of the display.

y_true=y,
y_pred=y_pred,
sample_weight=sample_weight,
cm = sklearn_confusion_matrix(
Copy link
Member

Choose a reason for hiding this comment

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

To follow the other display api, we should implement a _compute_data_for_display class method in the display. Basically, it will replace the previous from_predictions.


from skore.sklearn._plot.base import Display
from skore.sklearn._plot.style import StyleDisplayMixin
from skore.sklearn._plot.utils import HelpDisplayMixin
Copy link
Member

Choose a reason for hiding this comment

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

We should not decorate __init__ but the plot method (cf. RocCurveDisplay for a concrete example)



class ConfusionMatrixDisplay(Display):
class ConfusionMatrixDisplay(StyleDisplayMixin, HelpDisplayMixin):
Copy link
Member

Choose a reason for hiding this comment

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

display_labels, include_values, normalize and value_format should not be part of the constructor. They are way to change the plot and thus should be parameter of this plot method.

@glemaitre
Copy link
Member

Actually, the design is a bit more tricky.

I need to think a bit more about the design: normalize could be used to compute the data and should not be changeable at plot time. Then, we need to design the keywords such that it works properly with the StyleDisplayMixin. So we need to revisit the way that kwargs are passed.

@waridrox
Copy link
Contributor Author

Actually, the design is a bit more tricky.

I need to think a bit more about the design: normalize could be used to compute the data and should not be changeable at plot time. Then, we need to design the keywords such that it works properly with the StyleDisplayMixin. So we need to revisit the way that kwargs are passed.

Makes sense, I was on a long vacay, will look into this soon :)

@github-actions
Copy link
Contributor

github-actions bot commented Jun 16, 2025

Coverage

Coverage Report for skore/
FileStmtsMissCoverMissing
skore/src/skore
   __init__.py230100% 
   _config.py280100% 
   exceptions.py440%4, 15, 19, 23
skore/src/skore/project
   __init__.py20100% 
   project.py480100% 
   summary.py740100% 
   widget.py138596%375–377, 447–448
skore/src/skore/sklearn
   __init__.py70100% 
   _base.py1971492%45, 58, 126, 129, 182, 185–186, 188–191, 224, 227–228
   find_estimators.py270100% 
   find_ml_task.py610100% 
   types.py26196%26
skore/src/skore/sklearn/_comparison
   __init__.py50100% 
   metrics_accessor.py178398%167, 247, 1239
   report.py1050100% 
   utils.py540100% 
skore/src/skore/sklearn/_cross_validation
   __init__.py50100% 
   metrics_accessor.py182199%241
   report.py122199%466
skore/src/skore/sklearn/_estimator
   __init__.py70100% 
   feature_importance_accessor.py143298%216–217
   metrics_accessor.py365897%191, 193, 200, 291, 360, 364, 379, 414
   report.py164298%438–439
skore/src/skore/sklearn/_plot
   __init__.py20100% 
   base.py70100% 
   style.py290100% 
   utils.py118595%50, 74–76, 80
skore/src/skore/sklearn/_plot/metrics
   __init__.py60100% 
   confusion_matrix.py75494%97, 105, 131, 236
   metrics_summary_display.py90100% 
   precision_recall_curve.py278598%458, 558, 562, 622, 742
   prediction_error.py225597%180, 187, 423, 506, 686
   roc_curve.py290897%388, 511, 516, 619, 624, 628, 699, 821
skore/src/skore/sklearn/train_test_split
   __init__.py00100% 
   train_test_split.py580100% 
skore/src/skore/sklearn/train_test_split/warning
   __init__.py80100% 
   high_class_imbalance_too_few_examples_warning.py16193%79
   high_class_imbalance_warning.py170100% 
   random_state_unset_warning.py100100% 
   shuffle_true_warning.py9188%44
   stratify_is_set_warning.py100100% 
   time_based_column_warning.py21195%73
   train_test_split_warning.py30100% 
skore/src/skore/utils
   __init__.py6266%8, 13
   _accessor.py53296%68, 109
   _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.py460100% 
   _show_versions.py33293%65–66
   _testing.py550100% 
TOTAL34728997% 

Tests Skipped Failures Errors Time
931 5 💤 0 ❌ 0 🔥 7m 24s ⏱️

@waridrox waridrox force-pushed the refactorConfusionMatrix branch from e59f761 to bd2ecd7 Compare June 21, 2025 12:23
@waridrox
Copy link
Contributor Author

waridrox commented Jun 21, 2025

================================================== FAILURES ===================================================
____________________________________________ [xdoctest] Project:0 _____________________________________________
* REASON: SystemError
DOCTEST DEBUG INFO
  XDoc "/Users/m_warid/Desktop/dev/repos/skore/skore/src/skore/project/project.py::Project:0", line 35 <- wrt doctest
  File "/Users/m_warid/Desktop/dev/repos/skore/skore/src/skore/project/project.py", line 129, <- wrt source file
DOCTEST PART BREAKDOWN
Passed Parts:
     1 >>> from sklearn.datasets import make_classification, make_regression
     2 >>> from sklearn.linear_model import LinearRegression, LogisticRegression
     3 >>> from sklearn.model_selection import train_test_split
     4 >>> from skore.sklearn import EstimatorReport
     5 >>>
     6 >>> X, y = make_classification(random_state=42)
     7 >>> X_train, X_test, y_train, y_test = train_test_split(X, y, random_state=42)
     8 >>> classifier = LogisticRegression(max_iter=10)
     9 >>> classifier_report = EstimatorReport(
    10 >>>     classifier,
    11 >>>     X_train=X_train,
    12 >>>     y_train=y_train,
    13 >>>     X_test=X_test,
    14 >>>     y_test=y_test,
    15 >>> )
    16 >>>
    17 >>> X, y = make_regression(random_state=42)
    18 >>> X_train, X_test, y_train, y_test = train_test_split(X, y, random_state=42)
    19 >>> regressor = LinearRegression()
    20 >>> regressor_report = EstimatorReport(
    21 >>>     regressor,
    22 >>>     X_train=X_train,
    23 >>>     y_train=y_train,
    24 >>>     X_test=X_test,
    25 >>>     y_test=y_test,
    26 >>> )
Failed Part:
    30 >>> from pathlib import Path
    31 >>> from tempfile import TemporaryDirectory
    32 >>> from skore import Project
    33 >>>
    34 >>> tmpdir = TemporaryDirectory().name
    35 >>> local_project = Project("my-xp", workspace=Path(tmpdir))
Remaining Parts:
    39 >>> local_project.put("my-simple-classification", classifier_report)
    40 >>> local_project.put("my-simple-regression", regressor_report)
    44 >>> summary = local_project.summarize()
    45 >>> summary = summary.query("ml_task.str.contains('regression') and (rmse < 67)")
    46 >>> reports = summary.reports()
DOCTEST TRACEBACK
Traceback (most recent call last):

  File "/Users/m_warid/Desktop/dev/esoc/skore/displayAllMetrics/lib/python3.10/site-packages/xdoctest/doctest_example.py", line 868, in run
    exec(code, test_globals)

  File "<doctest:/Users/m_warid/Desktop/dev/repos/skore/skore/src/skore/project/project.py::Project:0>", line rel: 35, abs: 129, in <module>
    >>> local_project = Project("my-xp", workspace=Path(tmpdir))

  File "/Users/m_warid/Desktop/dev/repos/skore/skore/src/skore/project/project.py", line 199, in __init__
    mode, name, plugin, parameters = Project.__setup_plugin(name)

  File "/Users/m_warid/Desktop/dev/repos/skore/skore/src/skore/project/project.py", line 152, in __setup_plugin
    raise SystemError("No project plugin found, please install at least one.")

SystemError: No project plugin found, please install at least one.
DOCTEST REPRODUCTION
CommandLine:
    pytest /Users/m_warid/Desktop/dev/repos/skore/skore/src/skore/project/project.py::Project:0
/Users/m_warid/Desktop/dev/repos/skore/skore/src/skore/project/project.py:129: SystemError
-------------------------------------------- Captured stdout call ---------------------------------------------
====== <exec> ======
* DOCTEST : /Users/m_warid/Desktop/dev/repos/skore/skore/src/skore/project/project.py::Project:0, line 95 <- wrt source file
=========================================== short test summary info ===========================================
FAILED src/skore/project/project.py::Project:0

Is this a new addition to the testing suite? I had to run pip install skore-local-project prior to running tests in order to remove this error. Didn't see any mention of the same in the docs.

@MarieSacksick
Copy link
Contributor

skore-local-project should be included in the test dependencies imo, and not ask the user to install it.
@thomass-dev, is that correct?

@thomass-dev
Copy link
Collaborator

thomass-dev commented Jun 24, 2025

@waridrox please re-run make install-skore.
Consider re-executing this command each time you rebase your branch with main, as dependencies can change.

@waridrox
Copy link
Contributor Author

@waridrox please re-run make install-skore.

Thanks for the tip @thomass-dev, maybe worthwhile adding this to local development docs...

@waridrox waridrox requested a review from glemaitre June 24, 2025 19:00
@MarieSacksick
Copy link
Contributor

@waridrox : ongoing in this PR 083a91a :) !

@auguste-probabl auguste-probabl mentioned this pull request Jun 26, 2025
@MarieSacksick
Copy link
Contributor

@waridrox, do you want to continue this PR, or should we take it over and iterate from here?

@waridrox
Copy link
Contributor Author

waridrox commented Oct 8, 2025

Hi @MarieSacksick, I was waiting on some thoughts from @glemaitre on how to proceed about...

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.

api(skore): ConfusionMatrix is not following the Display API

4 participants