Skip to content

Conversation

@auguste-probabl
Copy link
Contributor

Replaces the checks in available_if with a check that at least one of the compared reports has the requested metric.

Closes #1473

@auguste-probabl auguste-probabl changed the title feat(ComparisonReport): Delegate available_if to sub-reports feat(ComparisonReport): Delegate available_if to sub-reports Sep 2, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2025

Coverage

Coverage Report for skore/
FileStmtsMissCoverMissing
skore/src/skore
   __init__.py230100% 
   _config.py310100% 
   exceptions.py440%4, 15, 19, 23
skore/src/skore/_sklearn
   __init__.py60100% 
   _base.py1981492%45, 58, 127, 130, 183, 186–187, 189–192, 225, 228–229
   find_ml_task.py610100% 
   types.py27196%28
skore/src/skore/_sklearn/_comparison
   __init__.py70100% 
   feature_importance_accessor.py39294%92, 111
   metrics_accessor.py178398%173, 253, 1215
   report.py1060100% 
   utils.py540100% 
skore/src/skore/_sklearn/_cross_validation
   __init__.py70100% 
   feature_importance_accessor.py240100% 
   metrics_accessor.py182199%244
   report.py135199%487
skore/src/skore/_sklearn/_estimator
   __init__.py90100% 
   data_accessor.py580100% 
   feature_importance_accessor.py144298%223–224
   metrics_accessor.py356897%200, 202, 209, 300, 369, 373, 388, 423
   report.py167298%448–449
skore/src/skore/_sklearn/_plot
   __init__.py30100% 
   base.py70100% 
   style.py290100% 
   utils.py141795%59, 83–85, 89, 344–345
skore/src/skore/_sklearn/_plot/data
   __init__.py20100% 
   table_report.py188199%684
skore/src/skore/_sklearn/_plot/metrics
   __init__.py60100% 
   confusion_matrix.py70494%91, 99, 121, 229
   feature_importance_display.py672168%92, 115–116, 118, 136–140, 142–149, 152–154, 156
   metrics_summary_display.py90100% 
   precision_recall_curve.py278598%459, 559, 563, 623, 743
   prediction_error.py225597%181, 188, 424, 507, 687
   roc_curve.py290897%389, 512, 517, 618, 623, 627, 696, 818
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.py90100% 
   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.py90396%34, 146, 190
   _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% 
   _repr_html.py80100% 
   _show_versions.py33293%65–66
   _testing.py550100% 
skore/src/skore/project
   __init__.py20100% 
   project.py480100% 
   summary.py740100% 
   widget.py165696%436, 439–441, 525–526
TOTAL393311697% 

Tests Skipped Failures Errors Time
1030 5 💤 0 ❌ 0 🔥 8m 50s ⏱️

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2025

Documentation preview @ 9839d48


def check(accessor: Any) -> bool:
return any(
hasattr(report.metrics, metric) for report in accessor._parent.reports_
Copy link
Collaborator

@thomass-dev thomass-dev Sep 3, 2025

Choose a reason for hiding this comment

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

Nitpick: you can factorize your function to check_any_sub_report_has_accessor with the following :

Suggested change
hasattr(report.metrics, metric) for report in accessor._parent.reports_
reduce(getattr, accessor.split("."), report)
for report in accessor._parent.reports_
@available_if(_check_any_sub_report_has_accessor("metrics.rmse"))

Copy link
Collaborator

@thomass-dev thomass-dev left a comment

Choose a reason for hiding this comment

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

Do you have any example in mind where the previous implementation would cause problems?
If so, please add a dedicated test, otherwise LGTM.

@auguste-probabl
Copy link
Contributor Author

Do you have any example in mind where the previous implementation would cause problems? If so, please add a dedicated test, otherwise LGTM.

I think this was the point of @guillaume that I split into #2001. That issue is proving hard to solve, so I could either add tests here and change them in #1985, or we could wait for #1985 (might be a while) and then merge this one.

@thomass-dev thomass-dev merged commit 4ca095e into probabl-ai:main Sep 5, 2025
34 checks passed
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.

chore: ComparisonReport should delegate to underlying estimator the availability of metrics

2 participants