feat(EstimatorReport): Display the feature permutation importance #1365
Conversation
auguste-probabl
changed the base branch from
main
to
1320-featestimatorreport-display-the-feature-weights-for-linear-models
auguste-probabl
force-pushed
the
1319-featestimatorreport-display-the-feature-permutation-importance
branch
from
e8d40ac to
475b6ef
Compare
auguste-probabl
force-pushed
the
1319-featestimatorreport-display-the-feature-permutation-importance
branch
from
475b6ef to
3ca3f74
Compare
skore/src/skore/sklearn/_estimator/feature_importance_accessor.py
Outdated
Show resolved
Hide resolved
|
The coverage report in the comment from the report I get locally (which is 100%)... |
skore/src/skore/sklearn/_estimator/feature_importance_accessor.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
It is a good start, I have the following remarks:
- I think we can limit the API of
scoringand only adopt what we do in thereport.metrics.report_metrics(). - Otherwise it is only little details.
I think that the caching is OKish but not the best. To make it better, we might need to revisit the implementation the scikit-learn implementation and cache at a different level.
If someone request the permutation importance (with a seed) and a single metric and then request another computation, we relaunch again the prediction computation of the model while it was computed the first time. When passing a list, the cache of the scikit-learn scorer will save us. But we don't deliver in a multiple call scenario.
To properly do it, we either need:
- to reimplement a good bunch of the permutation importance such that cache the prediction of the model, or
- there might be a dirty way to intervene with the cache of the scorer but it would not be easy (not sure about this one).
skore/src/skore/sklearn/_estimator/feature_importance_accessor.py
Outdated
Show resolved
Hide resolved
| n_jobs: Optional[int] = None, | ||
| random_state: Optional[Union[int, RandomState]] = None, | ||
| sample_weight: Optional[ArrayLike] = None, # Sample weights used in scoring. | ||
| max_samples: float = 1.0, |
There was a problem hiding this comment.
Let's move this one under n_repeats since it is more.
There was a problem hiding this comment.
sorry what do you mean?
skore/src/skore/sklearn/_estimator/feature_importance_accessor.py
Outdated
Show resolved
Hide resolved
skore/src/skore/sklearn/_estimator/feature_importance_accessor.py
Outdated
Show resolved
Hide resolved
skore/src/skore/sklearn/_estimator/feature_importance_accessor.py
Outdated
Show resolved
Hide resolved
| n_jobs=n_jobs, | ||
| random_state=random_state, | ||
| sample_weight=sample_weight, | ||
| max_samples=max_samples, |
There was a problem hiding this comment.
I think it makes sense to have an additional aggregate params to compute an aggregated score like the mean.
There was a problem hiding this comment.
The aggregation can happen after reloading from the cache because it will not be costly.
There was a problem hiding this comment.
We also need a flat_index to flatten the index if desired.
There was a problem hiding this comment.
added aggregation. I don't get if flat_index is necessary (take a look at the doctest)
| ) | ||
| score = pd.DataFrame(data=data, index=index, columns=columns) | ||
|
|
||
| # Unless random_state is an int (i.e. the call is deterministic), |
There was a problem hiding this comment.
It is a good point. I think we have a bug in the prediction_error plot when subsampling.
There was a problem hiding this comment.
yeah... caching is hard
| def case_several_scoring_numpy(): | ||
| data = regression_data() | ||
|
|
||
| kwargs = {"scoring": ["r2", "neg_root_mean_squared_error"], "random_state": 42} |
There was a problem hiding this comment.
I would not accept the neg_*** and be consistent with what we have already in the metrics report.
There was a problem hiding this comment.
that opens a can of worms. how do I know what metrics to accept? using _SCORE_OR_LOSS_INFO maybe?
| data = score | ||
| n_repeats = data.shape[1] | ||
| index = pd.Index(feature_names, name="Feature") | ||
| columns = pd.Index( |
There was a problem hiding this comment.
I would still think that we need to have the score because we don't know what are the repeat related to.
There was a problem hiding this comment.
If the user passes scoring=None, how do I know what metric was used?
auguste-probabl
force-pushed
the
1319-featestimatorreport-display-the-feature-permutation-importance
branch
2 times, most recently
from
39c86d5 to
48bee8b
Compare
auguste-probabl
force-pushed
the
1319-featestimatorreport-display-the-feature-permutation-importance
branch
from
d6f9fc3 to
9c8e914
Compare
auguste-probabl
force-pushed
the
1320-featestimatorreport-display-the-feature-weights-for-linear-models
branch
from
3ebfa52 to
69eb4f4
Compare
auguste-probabl
force-pushed
the
1319-featestimatorreport-display-the-feature-permutation-importance
branch
from
9c8e914 to
1c5166f
Compare
Closes #1319
Todo:
# TODOin codescoringis a callable (is it cached?)random_stateis aRandomStateinstance (is it cached?)random_stateis a RandomState then the call should not be cached, because reusing aRandomStategives a different result.