Skip to content

fix(preprocessor): reject an unrecognized scaling value - #41

Open
ChrisW09 wants to merge 1 commit into
mainfrom
fix/reject-unknown-scaling
Open

fix(preprocessor): reject an unrecognized scaling value#41
ChrisW09 wants to merge 1 commit into
mainfrom
fix/reject-unknown-scaling

Conversation

@ChrisW09

Copy link
Copy Markdown
Collaborator

Fixes #35.

Problem

resolve_method returns its input lowercased when it recognizes nothing, so an unrecognized
scaling value fell through the membership test and produced a pipeline with no scaler:

'minmax'        -> ['imputer', 'minmax', 'ple']
'zscore'        -> ['imputer', 'scaler', 'ple']
'not_a_scaler'  -> ['imputer', 'ple']          <- silently unscaled
'minmaxx'       -> ['imputer', 'ple']          <- silently unscaled

Preprocessor(scaling="minmaxx").fit(df, y) completed without complaint. Meanwhile the same
typo in numerical_method raises, a few lines further down in the same function.

Fix

Validate after alias resolution, keeping None and "none" as the documented ways to turn
scaling off:

InvalidParamError: get_numerical_transformer_steps.scaling = 'minmaxx' is invalid.
Constraint: must name a scaler, or be None / 'none' to disable scaling
Valid values: ['minmax', 'none', 'standardization']

Alias resolution is untouched, so "MinMax", "min-max", "zscore" and
"standardization" all keep working.

Why it matters more than a typical typo guard

The failure is invisible. The fit succeeds, the output widths are identical, and only the
numeric scale of the features differs — so the mistake surfaces, if at all, as a quietly
worse model rather than an error.

Tests

Nine added to tests/test_method_aliases.py: three unknown values that must raise, five
known spellings (including aliases) that must still insert their step, three disabling
spellings that must remain no-ops, and one end-to-end check through the Preprocessor.

Full suite: 492 passed, 9 xfailed. ruff check clean on changed files; pyright unchanged at
its pre-existing 71 errors.

🤖 Generated with Claude Code

``resolve_method`` returns its input lowercased when it recognizes nothing, so
an unknown ``scaling`` fell straight through the ``scaling in scalers``
membership test and produced a pipeline with no scaler at all:

    scaling='minmaxx'      -> ['imputer', 'ple']     silently unscaled
    scaling='not_a_scaler' -> ['imputer', 'ple']     silently unscaled

A typo therefore disabled scaling instead of reporting it, while the same typo
in ``method`` raises a few lines further down.

Validate after alias resolution, keeping ``None`` and ``"none"`` as the
documented ways to disable scaling.

This is worth guarding because the failure is invisible: the fit succeeds, the
output widths are unchanged, and only the numeric scale differs -- so the
mistake shows up, if at all, as a quietly worse model.

Closes #35

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

fix(preprocessor): reject an unrecognized scaling value

1 participant