Skip to content

Median-impute NaN in calc_cross_validation_score - #397

Open
drussellmrichie wants to merge 1 commit into
larsiusprime:masterfrom
drussellmrichie:fix/stats-cross-val-nan-imputation
Open

drussellmrichie wants to merge 1 commit into
larsiusprime:masterfrom
drussellmrichie:fix/stats-cross-val-nan-imputation

Conversation

@drussellmrichie

Copy link
Copy Markdown
Contributor

calc_elastic_net_regularization, calc_p_values_recursive_drop, calc_t_values_recursive_drop and calc_vif_recursive_drop all median-impute NaN before their sklearn / statsmodels fits (#313, #316, #318). calc_cross_validation_score was missed.

Real assessment data carries genuine missing building attributes — a few hundred null bedroom counts in a 500k-parcel universe is normal — so cross_val_score(LinearRegression(), X, y, cv=5) raises Input X contains NaN inside individual folds and emits hundreds of FitFailedWarnings per run.

The failure is silent as well as loud. When only some folds fail, -scores.mean() is nan, and the caller in model_runner.py:get_variable_recommendations does:

cv_score = calc_cross_validation_score(X, y)
if cv_score < best_score:      # nan < x is always False
    best_score = cv_score
    best_variables = curr_variables.copy()

so best_variables is never updated and the cross-validation refinement of the variable set is inert on any dataset with a NaN anywhere in X.

What this PR does

Adds the same median-impute guard and UserWarning the sibling helpers carry, plus two things they don't need:

  • all-NaN columns are dropped (there is no median to impute from), rather than letting a residual NaN fail the fit;
  • the np.ndarray half of the declared X: pd.DataFrame | np.ndarray signature is handled.

Behaviour on NaN-free input is unchanged (asserted in the test).

Test added: tests/test_stats.py::test_cross_validation_score_with_nan — NaN feature column, all-NaN column, and ndarray path.

Separate observation, deliberately not fixed here

This patch alone changes no model output, because the CV refinement is disconnected at both ends. get_variable_recommendations has exactly two call sites:

  1. try_variables calls it with do_cross=True, then does best_variables = var_recs["variables"] and never reads that variable again — one Store, zero Loads at model_runner.py:434, confirmed by AST rather than by eye. Only df_results escapes, to out/try/<group>/<status>.csv; the report is built but finish_report only runs when do_report=True.
  2. The auto-reduce inside run_one_model — the only path whose variables actually becomes ind_vars — passes do_cross=False.

So the CV loop runs where its answer is discarded, and is skipped where the answer would matter. (Confirmed downstream on our data: every model group's saved feature list is byte-identical to its configured ind_vars.)

Whether try_variables should use its own CV result, or whether auto-reduce should enable do_cross, changes which variables models actually train on — that seemed like your call rather than something to decide in a NaN-handling PR. Happy to follow up with whichever you prefer.

Found while running Philadelphia OPA data through the pipeline.

🤖 Generated with Claude Code

The sibling helpers in this file -- calc_elastic_net_regularization,
calc_p_values_recursive_drop, calc_t_values_recursive_drop and
calc_vif_recursive_drop -- all impute NaN before their sklearn /
statsmodels fits (PRs larsiusprime#313, larsiusprime#316, larsiusprime#318). calc_cross_validation_score was
missed.

Real assessment data carries genuine missing building attributes (a few
hundred null bedroom counts in a 500k-parcel universe is normal), so
cross_val_score raises "Input X contains NaN" inside individual folds and
emits hundreds of FitFailedWarnings per run.

The failure is also silent. When only some folds fail, -scores.mean() is
nan, and the caller in model_runner.py:get_variable_recommendations does:

    cv_score = calc_cross_validation_score(X, y)
    if cv_score < best_score:      # nan < x is always False
        best_score = cv_score
        best_variables = curr_variables.copy()

so best_variables is never updated and the cross-validation refinement of
the variable set is inert on any dataset with a NaN anywhere in X.

Adds a median-impute guard with the same UserWarning as the siblings,
drops all-NaN columns (no median to impute from), and supports the
np.ndarray half of the declared signature. Behaviour on NaN-free input is
unchanged.
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Thank you for your contribution.
Please sign our CLA at the following link:
Click here to sign the CLA.

No action is required from you in this PR thread. Once you have signed the CLA externally, a maintainer will verify your signature and record it here on your behalf by commenting:


I affirm that this contributor has signed the CLA


Russell Richie seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

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.

1 participant