Skip to content

Commit abd9f6f

Browse files
afpybusclaude
andcommitted
Keep permutation importance when coefficients fail; add importance_max_samples
Permutation importance came back empty for coxph, coxnet, fssvm, and weibull_aft whenever the pipeline's selector step exposed no selected_features_ (a chained feature_selector=[...] wraps a nested Pipeline, and third-party selectors do not have the attribute at all). sklearn's permutation_importance actually succeeded every time; the ValueError came from the coefficient step that ran right after it inside the same try, and the handler threw the computed importance away. Only those four models were affected because they are the only wrappers that override get_coefficients(); every other model short-circuits to a correct-length NaN list and never reaches the bad path. Splitting the work into two phases fixes that: a permutation failure still returns the empty sentinel, but a coefficient problem now degrades only the coefficient column to NaN. _get_coefficients is total, so it cannot hand a wrong-length list to the DataFrame builder, and _get_selected_features resolves names through a nested selector Pipeline and through get_feature_names_out(). The same work turned up a silent wrong answer. _align_coefficients keyed values positionally by the selector's order, but lifelines sorts Weibull-AFT covariates, so on the ordinary feature_selector='topk' path every weibull_aft coefficient landed on the wrong feature, sign flips included, in both the report and the eval_importance sheet. Coefficients are now keyed by the model's own index, which is what evaluation/_coefficients.py already did, so the two surfaces agree. Failures that remain are visible. log_metric_failure was resolving to InternalWarning, which configure_default_filters ignores by default, so the user saw a bare "failed (2.3s)." and nothing else. It now raises a MetricFailureWarning naming the model and the exception type, and the progress line carries the type too. Also adds importance_max_samples, an opt-in cap on the test rows used for permutation importance. The C-index term is superlinear in n_test, so on large cohorts importance can out-cost model fitting. Default None keeps every existing run byte-for-byte identical; the draw is seeded from random_state so models stay comparable, and an unseeded draw warns. Threaded through evaluate(), run(), tune(), and importance(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent fe6e58b commit abd9f6f

12 files changed

Lines changed: 1265 additions & 94 deletions

docs/feature-analysis.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,23 @@ print(result.evaluation.importance) # DataFrame: model_type, feature, coefficie
3535
learner.importance('rsf', method='permutation', n_importance_repeats=50)
3636
```
3737

38+
#### Cost on large cohorts
39+
40+
Permutation importance costs `n_features x n_repeats x (predict + C-index over n_test rows)`. The C-index term is superlinear in the number of test rows, so on large cohorts importance can take longer than fitting the models. Two levers:
41+
42+
```python
43+
# 1. Compute importance on a capped, seeded subsample of the test set.
44+
learner.setup(random_state=42) # the cap draws from this seed
45+
learner.evaluate(importance_max_samples=1000)
46+
47+
# 2. Skip it during an expensive sweep, then compute it on demand later,
48+
# including from a learner reloaded with load_learner().
49+
learner.evaluate(n_importance_repeats=0)
50+
learner.importance('coxph', method='permutation')
51+
```
52+
53+
`importance_max_samples` defaults to `None`, which uses every row. A value at or above the cohort size is a no-op, and the minimum is 2 (the C-index needs a pair of rows to compare). The draw is seeded from `random_state`, so with a seed set all features within a model, and all models in the call, are scored on the same rows and their importances stay comparable. Without a seed each model draws its own subsample; mlsurv warns when that happens. Permutation importance is a ranking diagnostic that tolerates subsampling well: the cost is a little precision in the importance estimate, not in the model.
54+
3855
### Grouped Importance
3956

4057
For one-hot encoded categorical features, grouped importance permutes all dummy columns jointly and reports a single aggregated score per original variable<sup>[2](#references),[3](#references)</sup>:

mlsurv/evaluation/_config.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@ class EvalConfig:
5252
call layers unchanged.
5353
"""
5454
n_importance_repeats: int = 10
55+
# Cap on the test rows used for permutation importance. None = all rows.
56+
importance_max_samples: Optional[int] = None
5557
eval_times: Optional[np.ndarray] = None
5658
random_state: Optional[int] = None
5759
include_shap: bool = False
@@ -81,6 +83,20 @@ class EvalConfig:
8183
gnd_n_bins: int = 10
8284

8385
def __post_init__(self):
86+
if self.importance_max_samples is not None:
87+
if (isinstance(self.importance_max_samples, bool)
88+
or not isinstance(self.importance_max_samples,
89+
(int, np.integer))):
90+
raise ValueError(
91+
f"importance_max_samples must be an int or None, got "
92+
f"{type(self.importance_max_samples).__name__}."
93+
)
94+
# Lower bound of 2 explained in _subsample_for_importance.
95+
if self.importance_max_samples < 2:
96+
raise ValueError(
97+
f"importance_max_samples must be >= 2 or None, got "
98+
f"{self.importance_max_samples}."
99+
)
84100
if self.include_shap:
85101
if self.shap_max_background < 1:
86102
raise ValueError(

mlsurv/evaluation/evaluate.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,7 @@ def evaluate(
243243
eval_times=None,
244244
extra_eval_times=None,
245245
n_importance_repeats: Union[int, str] = "auto",
246+
importance_max_samples: Optional[int] = None,
246247
include_shap=False,
247248
shap_max_background=50,
248249
shap_max_explain=50,
@@ -318,6 +319,12 @@ def evaluate(
318319
``"auto"`` (the default), uses 10 repeats for all models.
319320
Pass a dict of {model_name: int} for per-model repeat counts;
320321
models not in the dict fall back to ``"auto"``.
322+
importance_max_samples : int, optional
323+
Cap on the test rows used for permutation importance. ``None``
324+
(default) uses every row. A value at or above the cohort size is a
325+
no-op; the minimum is 2. The draw is seeded from *random_state*, so
326+
with a seed set every model sees the same rows; without one, each
327+
model draws its own subsample and mlsurv warns.
321328
include_shap : bool, default=False
322329
If True, compute SHAP values (requires shap package).
323330
shap_max_background : int, default=50
@@ -456,6 +463,7 @@ def evaluate(
456463
eval_times=eval_times,
457464
extra_eval_times=extra_eval_times,
458465
n_importance_repeats=resolved_repeats,
466+
importance_max_samples=importance_max_samples,
459467
include_shap=include_shap,
460468
shap_max_background=shap_max_background,
461469
shap_max_explain=shap_max_explain,
@@ -514,6 +522,7 @@ def evaluate(
514522
eval_times=eval_times,
515523
extra_eval_times=extra_eval_times,
516524
n_importance_repeats=single_repeats,
525+
importance_max_samples=importance_max_samples,
517526
include_shap=include_shap,
518527
shap_max_background=shap_max_background,
519528
shap_max_explain=shap_max_explain,
@@ -551,6 +560,7 @@ def _evaluate_single(
551560
eval_times=None,
552561
extra_eval_times=None,
553562
n_importance_repeats=DEFAULT_N_IMPORTANCE_REPEATS,
563+
importance_max_samples=None,
554564
include_shap=False,
555565
shap_max_background=50,
556566
shap_max_explain=50,
@@ -662,6 +672,7 @@ def _evaluate_single(
662672
# Bundle eval config for helper calls
663673
config = EvalConfig(
664674
n_importance_repeats=n_importance_repeats,
675+
importance_max_samples=importance_max_samples,
665676
eval_times=eval_times,
666677
random_state=random_state,
667678
include_shap=include_shap,
@@ -861,6 +872,7 @@ def _run_importance_phase(model, X_test, y_test, config, model_name,
861872
show_progress=config.show_progress, model_name=model_name,
862873
n_jobs=importance_n_jobs,
863874
categorical_groups=config.categorical_groups,
875+
max_samples=config.importance_max_samples,
864876
)
865877
else:
866878
importance_df, iterations_df = _empty_importance_results()
@@ -1695,6 +1707,7 @@ def _evaluate_multiple(
16951707
eval_times=None,
16961708
extra_eval_times=None,
16971709
n_importance_repeats=DEFAULT_N_IMPORTANCE_REPEATS,
1710+
importance_max_samples=None,
16981711
include_shap=False,
16991712
shap_max_background=50,
17001713
shap_max_explain=50,
@@ -1821,6 +1834,7 @@ def _evaluate_multiple(
18211834
eval_times=eval_times,
18221835
extra_eval_times=extra_eval_times,
18231836
n_importance_repeats=model_repeats,
1837+
importance_max_samples=importance_max_samples,
18241838
include_shap=include_shap,
18251839
shap_max_background=shap_max_background,
18261840
shap_max_explain=shap_max_explain,

0 commit comments

Comments
 (0)