test: Include all estimators (with coef_) in test_all_sklearn_estimators
#1575
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
I think it's okay! |
Schefflera-Arboricola
changed the title
coef_) while testing EstimatorReport.feature_importance.coefficients()coef_) while testing EstimatorReport.feature_importance.coefficients()
Coverage Report for backend
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Schefflera-Arboricola
changed the title
coef_) while testing EstimatorReport.feature_importance.coefficients()coef_) in test_all_sklearn_estimators
I'd say yes! |
glemaitre
reviewed
Apr 23, 2025
There was a problem hiding this comment.
Thanks @Schefflera-Arboricola
Just a couple of comments and questions.
skore/src/skore/sklearn/_estimator/feature_importance_accessor.py
Outdated
Show resolved
Hide resolved
skore/tests/unit/sklearn/estimator/feature_importance/conftest.py
Outdated
Show resolved
Hide resolved
skore/tests/unit/sklearn/estimator/feature_importance/conftest.py
Outdated
Show resolved
Hide resolved
skore/tests/unit/sklearn/estimator/feature_importance/test_coefficients.py
Outdated
Show resolved
Hide resolved
skore/tests/unit/sklearn/estimator/feature_importance/test_coefficients.py
Outdated
Show resolved
Hide resolved
skore/tests/unit/sklearn/estimator/feature_importance/test_coefficients.py
Outdated
Show resolved
Hide resolved
glemaitre
changed the title
coef_) in test_all_sklearn_estimatorscoef_) in test_all_sklearn_estimators
… SGDOneClassSVM in coefficients test
…d its test and coefficients() accordingly; included TransformedTargetRegressor
Co-authored-by: Guillaume Lemaitre <[email protected]>
…s, testing signing commits
Co-authored-by: Auguste Baum <[email protected]>
Schefflera-Arboricola
force-pushed
the
issues_1364
branch
from
b277a50 to
6777423
Compare
auguste-probabl
approved these changes
Apr 28, 2025
Coverage Report for |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File | Stmts | Miss | Cover | Missing |
|---|---|---|---|---|
| venv/lib/python3.12/site-packages/skore | ||||
| __init__.py | 22 | 0 | 100% | |
| _config.py | 28 | 0 | 100% | |
| exceptions.py | 4 | 4 | 0% | 4–23 |
| venv/lib/python3.12/site-packages/skore/persistence | ||||
| __init__.py | 0 | 0 | 100% | |
| venv/lib/python3.12/site-packages/skore/persistence/item | ||||
| __init__.py | 55 | 1 | 98% | 97 |
| altair_chart_item.py | 19 | 1 | 91% | 14 |
| item.py | 22 | 1 | 95% | 86 |
| matplotlib_figure_item.py | 36 | 1 | 95% | 19 |
| media_item.py | 22 | 0 | 100% | |
| numpy_array_item.py | 27 | 1 | 94% | 16 |
| pandas_dataframe_item.py | 29 | 1 | 94% | 14 |
| pandas_series_item.py | 29 | 1 | 94% | 14 |
| pickle_item.py | 22 | 0 | 100% | |
| pillow_image_item.py | 25 | 1 | 93% | 15 |
| plotly_figure_item.py | 20 | 1 | 92% | 14 |
| polars_dataframe_item.py | 27 | 1 | 94% | 14 |
| polars_series_item.py | 22 | 1 | 92% | 14 |
| primitive_item.py | 23 | 2 | 91% | 13–15 |
| sklearn_base_estimator_item.py | 29 | 1 | 94% | 15 |
| venv/lib/python3.12/site-packages/skore/persistence/repository | ||||
| __init__.py | 2 | 0 | 100% | |
| item_repository.py | 59 | 5 | 91% | 15–16, 202–203, 226 |
| venv/lib/python3.12/site-packages/skore/persistence/storage | ||||
| __init__.py | 4 | 0 | 100% | |
| abstract_storage.py | 22 | 0 | 100% | |
| disk_cache_storage.py | 33 | 1 | 95% | 44 |
| in_memory_storage.py | 20 | 0 | 100% | |
| venv/lib/python3.12/site-packages/skore/project | ||||
| __init__.py | 2 | 0 | 100% | |
| project.py | 83 | 2 | 98% | 280, 392 |
| venv/lib/python3.12/site-packages/skore/sklearn | ||||
| __init__.py | 6 | 0 | 100% | |
| _base.py | 171 | 14 | 92% | 45, 58, 126, 129, 182–191, 203–>209, 224, 227–228 |
| find_ml_task.py | 61 | 0 | 99% | 136–>145 |
| types.py | 13 | 0 | 100% | |
| venv/lib/python3.12/site-packages/skore/sklearn/_comparison | ||||
| __init__.py | 5 | 0 | 100% | |
| metrics_accessor.py | 165 | 2 | 97% | 163, 164–>166, 1278 |
| report.py | 67 | 1 | 97% | 17, 249–>252 |
| venv/lib/python3.12/site-packages/skore/sklearn/_cross_validation | ||||
| __init__.py | 5 | 0 | 100% | |
| metrics_accessor.py | 190 | 0 | 99% | 153–>155, 155–>157 |
| report.py | 110 | 1 | 98% | 23 |
| venv/lib/python3.12/site-packages/skore/sklearn/_estimator | ||||
| __init__.py | 7 | 0 | 100% | |
| feature_importance_accessor.py | 143 | 0 | 99% | 497–>503, 583–>592 |
| metrics_accessor.py | 344 | 10 | 96% | 174–183, 211–>220, 219, 249, 260–>262, 290, 317–321, 336, 371, 372–>374 |
| report.py | 148 | 1 | 98% | 24, 253–>255 |
| venv/lib/python3.12/site-packages/skore/sklearn/_plot | ||||
| __init__.py | 2 | 0 | 100% | |
| base.py | 6 | 0 | 100% | |
| style.py | 28 | 0 | 100% | |
| utils.py | 122 | 5 | 95% | 51, 75–77, 81 |
| venv/lib/python3.12/site-packages/skore/sklearn/_plot/metrics | ||||
| __init__.py | 4 | 0 | 100% | |
| precision_recall_curve.py | 173 | 1 | 99% | 660 |
| prediction_error.py | 164 | 0 | 100% | |
| roc_curve.py | 176 | 1 | 99% | 649 |
| venv/lib/python3.12/site-packages/skore/sklearn/train_test_split | ||||
| __init__.py | 0 | 0 | 100% | |
| train_test_split.py | 51 | 1 | 96% | 16, 154–>158 |
| 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 | 90% | 80 |
| high_class_imbalance_warning.py | 18 | 0 | 100% | |
| random_state_unset_warning.py | 12 | 1 | 88% | 15 |
| shuffle_true_warning.py | 10 | 1 | 83% | 46 |
| stratify_is_set_warning.py | 12 | 1 | 88% | 15 |
| time_based_column_warning.py | 23 | 2 | 86% | 17, 73 |
| train_test_split_warning.py | 4 | 0 | 100% | |
| venv/lib/python3.12/site-packages/skore/utils | ||||
| __init__.py | 6 | 0 | 100% | |
| _accessor.py | 52 | 2 | 93% | 63–>68, 67, 108 |
| _environment.py | 27 | 0 | 97% | 30–>35 |
| _fixes.py | 8 | 0 | 100% | |
| _index.py | 5 | 0 | 100% | |
| _logger.py | 22 | 4 | 85% | 15–19 |
| _measure_time.py | 10 | 0 | 100% | |
| _parallel.py | 38 | 3 | 88% | 23–33, 124 |
| _patch.py | 13 | 5 | 53% | 21–37 |
| _progress_bar.py | 36 | 0 | 100% | |
| _show_versions.py | 33 | 0 | 100% | |
| TOTAL | 3201 | 83 | 96% | |
| Tests | Skipped | Failures | Errors | Time |
|---|---|---|---|---|
| 816 | 8 💤 | 0 ❌ | 0 🔥 | 53.373s ⏱️ |
auto-merge was automatically disabled
April 28, 2025 13:27
Pull Request is not mergeable
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 30, 2025
followup for PR #1575
Muhammad-Rebaal
pushed a commit
to Muhammad-Rebaal/skore
that referenced
this pull request
May 5, 2025
…mators` (probabl-ai#1575) closes issue probabl-ai#1364 - incorporated: - [x] sklearn.linear_model.GammaRegressor() - [x] sklearn.linear_model.PoissonRegressor() - [x] sklearn.compose.TransformedTargetRegressor() - [ ] sklearn.cross_decomposition.CCA() - [ ] sklearn.cross_decomposition.PLSCanonical() - [ ] sklearn.linear_model.MultiTaskElasticNet() - [ ] sklearn.linear_model.MultiTaskElasticNetCV() - [ ] sklearn.linear_model.MultiTaskLasso() - [ ] sklearn.linear_model.MultiTaskLassoCV() - [ ] sklearn.svm.OneClassSVM() - [ ] sklearn.linear_model.SGDOneClassSVM() - other changes: - [x] handling estimators that does not support `coef_` and/or `intercept_` when creating `coefficients()` in EstimatorReport - [x] removed sklearn.linear_model.Lasso() --> duplicate entry - [x] updated `_check_has_coef` and `coefficients()` to handle cases of meta estimators (updated tests accordingly) - [x] reorganised the commented models - need feedback on: - [x] is there a better(cleaner) way to get data? -- right now I've created different fixtures for `positive_regression_data`, `multi_regression_data`, `outlier_data` and `clustering_data` -- but all these are only used once in this one test (i.e. `test_all_sklearn_estimators`). - [x] for `TransformedTargetRegressor` should we modify the `_check_has_coef` in `skore/src/skore/utils/_accessor.py` to not just check if the `hasattr(estimator, "coef_")` is true but also check if `hasattr(estimator.regressor_, "coef_")` is also true ([here](https://github.com/probabl-ai/skore/blob/main/skore/src/skore/utils/_accessor.py#L60))? or should we exclude `TransformedTargetRegressor` from this test? - [x] even though the tests passed locally, I'm not sure about how I've incorporated `sklearn.cross_decomposition.CCA()` and `sklearn.cross_decomposition.PLSCanonical()` - [ ] any other feedback that you would like to give! Thanks :) --------- Co-authored-by: Guillaume Lemaitre <[email protected]> Co-authored-by: Auguste Baum <[email protected]>
Muhammad-Rebaal
pushed a commit
to Muhammad-Rebaal/skore
that referenced
this pull request
May 5, 2025
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.
closes issue #1364
incorporated:
other changes:
coef_and/orintercept_when creatingcoefficients()in EstimatorReport_check_has_coefandcoefficients()to handle cases of meta estimators (updated tests accordingly)need feedback on:
positive_regression_data,multi_regression_data,outlier_dataandclustering_data-- but all these are only used once in this one test (i.e.test_all_sklearn_estimators).TransformedTargetRegressorshould we modify the_check_has_coefinskore/src/skore/utils/_accessor.pyto not just check if thehasattr(estimator, "coef_")is true but also check ifhasattr(estimator.regressor_, "coef_")is also true (here)? or should we excludeTransformedTargetRegressorfrom this test?sklearn.cross_decomposition.CCA()andsklearn.cross_decomposition.PLSCanonical()Thanks :)