fix: resolve 10 high-priority bugs found in a library audit (#76-#85) by ChrisW09 · Pull Request #86 · OpenTabular/PreTab

ChrisW09 · 2026-10-09T13:20:33Z

This PR fixes the 10 most important bugs from a new audit of the library. It is stacked on #75 and targets fix/open-bug-batch, so this diff shows only the new commits. After #75 is merged, change the base to main (or delete fix/open-bug-batch, which retargets automatically).

How the bugs were selected

I audited five areas of the code:

numerical encodings and the edge-case policy
feature maps and kernel approximation
splines
the Preprocessor / composition layer
categorical encodings, extension and serialization

That produced about 38 candidates. Each one was reproduced with a script that I ran, and each of the 10 selected bugs was reproduced again on main (6bacaba). Candidates were ranked by impact × likelihood:

impact: silent wrong results, then crashes on supported usage, then contract or metadata issues
likelihood: default path, then common opt-in, then niche

Each bug has its own issue with a reproduction, its own commit, and regression tests. Every new test set fails on the parent commit and passes here.

Fixes

#	Issue	Fix
1	#79 `CrossFittedTransformer(Preprocessor)` refit column types and category codes per fold, so out-of-fold features were encoded differently from `transform`	A wrapped `Preprocessor` now derives each fold model from the all-data fit and refits only the target-aware blocks (`Preprocessor._cross_fit_fold`). Out-of-fold rows use exactly the encoding of `transform`, and target-aware placement still never sees a row's own target. Other estimators keep the clone-and-refit path. Sparse fold output is densified, dict output raises a clear error, and the width-mismatch message no longer blames adaptive sizing only.
2	#76 One-hot encoding and `preset="expanded"` crashed under `sklearn.set_config(transform_output="pandas")`	The internal `ColumnTransformer` is pinned to `set_output(transform="default")`. The Preprocessor still wraps its own output. This also stops the #75 `MissingIndicator` branch from producing an object-dtype frame under that setting.
3	#77 `RepresentationSearchCV` with a classifier kept `task="regression"`	A classifier defaults the preprocessors to `task="classification"`. An explicit `task` in `preprocessor_params` still wins.
4	#78 A DataFrame-fitted `Preprocessor` rejected a same-width NumPy array at `transform`, and vice versa	An array of the fitted width after a DataFrame fit, or a DataFrame after an array fit, is matched by position, with scikit-learn's usual feature-name warning. Numeric columns of object arrays are re-inferred. A different width still raises.
5	#84 A partial `policy` mapping switched the clip-family splines to `"extrapolate"`, and `"extrapolate"` / `"warn"` returned all-zero rows	A mapping now overrides only the axes it names, on top of the family defaults; an unknown axis raises a typed error. Out-of-range rows extend the boundary polynomial pieces: `BSpline(extrapolate=True)` for B/M-splines, and the out-of-range rows of the P-spline / tensor-product basis. In-range values are unchanged.
6	#85 Standalone numerical transformers dropped DataFrame feature names and silently accepted reordered columns	The shared validator and the PLE / binning validation record and check feature names with scikit-learn's `_check_feature_names`. `get_feature_names_out()` builds names from `feature_names_in_` and validates `input_features`. scikit-learn's `check_dataframe_column_names_consistency` and `check_transformer_get_feature_names_out_pandas` now pass.
7	#80 Default `to_spec()` files contained bare `NaN` tokens and were not valid JSON for strict parsers	Non-finite floats (scalars and array elements) are tagged as `{"__float__": "nan"}` and specs are dumped with `allow_nan=False`. Specs written by `main` still load and reproduce bit-for-bit (verified with a spec written by `main`).
8	#81 `knot_locations` was still sized by `output_dim`	Explicit knots fix the width at `len + degree + 1` and bypass `output_dim` and the adaptive bounds. Repeated knots are merged. Knots on or outside the range are dropped with a `DataWarning`. Malformed input raises `InvalidParamError`.
9	#82 Unsupervised quantile placement repeated feature-map centers on tied data, producing duplicate columns	`QuantilePlacement` returns distinct locations: the endpoints are kept once and the interior is topped up with strictly interior candidates, using the same helper as the splines. Untied data and constant columns are unchanged.
10	#83 Unsupervised ReLU placed its last center at the training maximum, giving a dead column	ReLU places `output_dim + 1` range-spanning locations and drops the right endpoint. RBF / sigmoid / tanh and the target-aware path are unchanged.

Follow-up commits: regressions found by the next audit

The audit behind #97, and a review of it, found three regressions introduced by this PR and #75. They are fixed by the last three commits here, so none of them reaches main:

Commit	Problem	Fix
`fix(representation): resolve spec input names like get_feature_names_out`	The #85 fix made `get_feature_names_out(input_features)` require the fitted column names, but `get_representation_spec()` still passed `x0, x1, ...`. It raised `input_features is not equal to feature_names_in_` for every numerical transformer fitted on a DataFrame. The Preprocessor's spec summary swallowed that error, so `to_spec()["representations"]` and `reproducibility_report()` came back empty whenever a representation was fitted directly on its column.	The spec resolves its input names like `get_feature_names_out`. The `CrossFittedTransformer` fallback uses the wrapped estimator's names. The summary passes each block's column names and no longer hides errors; a leaf fitted on more inputs (1.0.0 objects with `add_missing_indicator`) names its own. `get_feature_info()` probes with the fitted names, so it no longer warns.
`fix(serialization): unwrap missing-indicator blocks in the representation summary`	Since the #62 fix in #75, indicator blocks are a `FeatureUnion`, and the summary treated the union as the leaf. So `add_missing_indicator=True` / `impute_with_indicator` listed no representations (`separate_state` never did), and the `verbose=3` log skipped them.	A `representation_leaf()` helper unwraps the representation branch for the summary and for the log.
`fix(cross-fitting): refit registered target-using classes on every fold`	The #79 fix refits only the target-aware blocks of a wrapped `Preprocessor` per fold, and recognised them only from a `RepresentationSpec` or a fixed list of scikit-learn step names. A class registered with `supervision="supervised"` / `"optional"` that has no spec (for example scikit-learn's `TargetEncoder`) was treated as target-free, so `CrossFittedTransformer` encoded every out-of-fold row with the all-data fit that had seen the row's own target. On a pure-noise target the out-of-fold encoding correlated with `y` at about 0.5. On `main` the whole Preprocessor was refit per fold, so this is a regression.	Such a block is resolved from its registry entry: a `"supervised"` class, or an `"optional"` one with `target_aware` set, counts as target-aware, so it is refit on every fold and lineage reports `uses_target`.

Behaviour changes from these commits:

Summary entries now use the block's column names (age_rbf0 instead of x0_rbf0).
separate_state blocks now appear in the summary.
A spec that fails to build now raises instead of being dropped.
fingerprint_ does not hash the summary, so it is unchanged by these commits.
Registered target-using classes without a RepresentationSpec are refit per fold by CrossFittedTransformer and reported with uses_target=True in the lineage.

Each commit has regression tests that fail on its parent. A spec and a pickle written by 1.0.0 with add_missing_indicator / impute_with_indicator still give to_spec, fingerprint_ and reproducibility_report.

Behaviour changes to review

ReLU (unsupervised path): centers move from linspace(min, max, k) to linspace(min, max, k + 1)[:-1] (and the quantile analogue). Width is unchanged.
Quantile placement: on tied data the interior locations change. On untied data they are bit-identical.
Explicit spline policies: an explicit RepresentationPolicy(out_of_range="extrapolate" | "warn") now produces extrapolated B/M/P/tensor rows instead of zeros. Values grow quickly far outside the range, as polynomial extrapolation does. The default stays "clip".
Standalone transformers fitted on a DataFrame:
- output names now use the column names (age_rbf0 instead of x0_rbf0);
- reordered or renamed columns raise;
- an array at transform warns, as in scikit-learn.
Preprocessor output names are unchanged.
Fingerprints: fingerprint_ changes for every newly fitted preprocessor: the internal ColumnTransformer now records its pinned output setting (Preprocessor with one-hot encoding (and preset="expanded") crashes under sklearn.set_config(transform_output="pandas") #76), the spec format tags non-finite floats (Default to_spec() files contain bare NaN tokens and are not valid JSON for strict parsers #80), and a representation fitted directly on a frame column records feature_names_in_ (Standalone numerical transformers drop DataFrame feature names: reordered columns are silently accepted and outputs are named x0_... #85). A preprocessor loaded from a 1.0.0 spec or pickle whose state holds a NaN (every one with an imputer) also reports a new fingerprint_; its output is unchanged.
The golden regression baselines pass unchanged.

Testing

Full suite: 1726 passed, 59 skipped, 7 xfailed with the three follow-up commits (1690 without them). fix: resolve 18 open bug reports across splines, placement and Preprocessor #75's head has 1591 in the same environment: Python 3.12, numpy 2.5.3, pandas 2.3.3, scikit-learn 1.9.1, scipy 1.18.1, lightgbm 4.7.0.
Minimum dependencies (CI job: Python 3.10, numpy 1.24.4, pandas 2.0.3, scipy 1.10.1, scikit-learn 1.6.0): 1703 passed, 82 skipped with the follow-up commits (1667 without them), and scripts/quickstart.py passes.
Coverage: 94.22% with the follow-up commits (94.12% without them; the gate is 90%).
Lint and types: ruff check and ruff format --check are clean. pyright reports only the 6 errors that also appear on main in this environment.
Reproductions: the script in each of the 10 issues was re-run on this branch and now shows the fixed behaviour.

Not included (confirmed, lower priority, or design changes)

Design questions rather than bug fixes, because each would change documented defaults for every user:
- RBF, sigmoid and tanh use a fixed absolute width (gamma=1 / scale=1), so extra output_dim adds little capacity.
- Fourier features use the observed range as the period, so the minimum and maximum encode identically.
- The cubic-regression and natural-cubic bases are built on raw x, which makes them unit-dependent and ill-conditioned for years or timestamps.
Lower-priority confirmed items:
- Thin-plate omits its linear null space.
- Soft binning with ±inf edges gives NaN.
- Default PLE output is float32 although dtype=None is documented as float64.
- Name cleaning can create duplicate output names.
- A failed fit leaves the lifecycle state at FITTED.
- Column-type detection near cat_cutoff can flip between RepresentationSearchCV folds and the refit.
- Some check_representation and embedding-backend contract issues.

Closes #76, closes #77, closes #78, closes #79, closes #80, closes #81, closes #82, closes #83, closes #84, closes #85

A global sklearn.set_config(transform_output="pandas") (or "polars") reached the steps of the internal ColumnTransformer. The sparse OneHotEncoder refuses pandas output, so categorical_method="one-hot" and preset="expanded" failed to fit, and mixed float / bool blocks (the missing indicator) were stacked into an object-dtype frame. The internal ColumnTransformer now always produces plain arrays; the Preprocessor still wraps its stacked output in the requested container itself. Fixes #76

…fier RepresentationSearchCV stratified its folds for a classifier but built every candidate Preprocessor with the default task="regression". Target-aware placement then crashed on string class labels and treated integer labels as a regression target. A classifier now defaults the preprocessors to task="classification"; an explicit task in preprocessor_params still wins. Fixes #77

Array input was always relabelled feature_0, feature_1, ... and routed by name, so a Preprocessor fitted on a DataFrame raised 'columns are missing' for the same data as a NumPy array (NumPy-based serving, shap.KernelExplainer), and one fitted on an array raised for a DataFrame. Following scikit-learn's convention, input of the fitted width that does not carry the fitted labels is now matched by position (numeric columns of an object array are re-inferred) with scikit-learn's usual feature-name warning; input of a different width still raises. Fixes #78

…reprocessor per fold CrossFittedTransformer refit a fresh clone of the wrapped estimator on each fold. For a Preprocessor that re-detected column types (the cat_cutoff ratio depends on the row count) and relearned category vocabularies on fewer rows, so the out-of-fold training features used different integer codes, a different numerical/categorical split, or a different one-hot width than transform. A Preprocessor now derives each fold model from the all-data fit and refits only the blocks that consume the target; everything else is shared, so out-of-fold rows are encoded exactly like transform encodes them while target-aware placement still never sees a row's own target. Other estimators keep the clone and refit path. Sparse fold output is densified, dict output raises a clear error, and the width-mismatch message no longer blames adaptive sizing only. Fixes #79

… tokens The imputers' missing_values=np.nan (and any other non-finite float in fitted state) was written as a bare NaN token, so every default spec file was rejected by strict JSON parsers outside Python. Non-finite floats, as scalars and as float-array elements, are now encoded as a tagged value and decoded back, and specs are dumped with allow_nan=False. Specs written by earlier versions still load. Fixes #80

BaseSplineTransformer documented that output_dim is ignored when knot_locations is given, but it still checked the knot count against output_dim - degree - 1 (raising for any other count), trimmed the knots to the adaptive window, and clipped out-of-range knots onto the boundary. Explicit knots now fix the width (len(knot_locations) + degree + 1 basis functions per feature) and bypass output_dim and the adaptive bounds. Repeated knots are merged, knots on or outside a feature's fitted range are dropped with a DataWarning, and knot_locations must be a 1-D sequence of finite numbers. Fixes #81

QuantilePlacement returned the data quantiles verbatim, so on zero-inflated, top-coded or discrete features the RBF / ReLU / sigmoid / tanh feature maps got repeated centers and exactly duplicated output columns (4 of 8 distinct on a zero-inflated feature). The locations are now made distinct: endpoints are kept once and the rest is topped up with strictly interior quantile / uniform candidates, as the splines already do. Untied data is unchanged, and a zero-range feature keeps its repeated location so the width holds. Fixes #82

…aining maximum On the uniform / quantile path the ReLU expansion reused the feature maps' endpoint-inclusive centers, so its last ramp max(0, x - c) started at the training maximum and was identically zero on every training row. The ReLU now places output_dim + 1 range-spanning locations and drops the right endpoint, so every column carries signal; RBF / sigmoid / tanh and the target-aware path are unchanged. Fixes #83

…xtrapolate out-of-range rows A policy mapping such as {"constant": "warn"} was expanded with the dataclass defaults, so it reset out_of_range to "extrapolate" and dropped the clip default of the B/M/I-spline, P-spline and tensor-product families. Under "extrapolate" / "warn" those bases then evaluated to zero outside the knot span, turning every out-of-range row into an all-zero basis row. A mapping now overrides only the axes it names on top of the family defaults (an unknown axis raises a typed error; an instance is still used verbatim), and out-of-range rows are evaluated by extending the boundary polynomial pieces (BSpline(extrapolate=True) for B/M-splines, and for the out-of-range rows of the P-spline / tensor-product basis). In-range values are unchanged. Fixes #84

…it-learn The standalone numerical transformers validated input with check_array only, so fitting on a DataFrame recorded no feature_names_in_: a reordered or renamed frame was processed by position (silently wrong features), output names were x0_..., and PLE / binning / tensor-product get_feature_names_out accepted an input_features of the wrong length. The shared validator, and the PLE and binning validation, now record feature_names_in_ at fit and check the column names at transform via scikit-learn's _check_feature_names (renamed or reordered columns raise, missing names warn). get_feature_names_out() resolves its prefixes from feature_names_in_ (else x0, x1, ...) and validates input_features. The binning feature-count error now uses scikit-learn's message. Fixes #85

The #85 fix (record and check DataFrame feature names like scikit-learn) made get_feature_names_out(input_features) require the fitted column names, but get_representation_spec() still built x0, x1, ... itself and passed them on. So it raised "input_features is not equal to feature_names_in_" for every transformer fitted on a DataFrame. The Preprocessor's spec summary swallowed that error, so to_spec() and reproducibility_report() listed no representations whenever one was fitted directly on its column (no imputer or scaler), and get_feature_info() warned that X had no valid feature names. get_representation_spec() now resolves its input names like get_feature_names_out (explicit names validated, else feature_names_in_, else x0, x1, ...). The CrossFittedTransformer fallback uses the wrapped estimator's feature_names_in_, which also fixes a wrapped Preprocessor fitted on a frame. The summary passes each block's column names (a leaf fitted on more inputs, as before the #62 fix, names its own), so its entries read age_rbf0 instead of x0_rbf0, and it no longer hides spec errors. get_feature_info() probes a step with a frame carrying its fitted names. Follow-up to #85

…tion summary The #62 fix (keep the imputer's missing indicator out of the representation) builds add_missing_indicator=True and missing_policy='impute_with_indicator' blocks as a FeatureUnion of the representation pipeline and a MissingIndicator, as missing_policy='separate_state' already did. The spec summary read each block's last step, which for such a block is the union itself, so to_spec() and reproducibility_report() listed no representation for them (separate_state was already empty before), and the verbose=3 log skipped their fitted thresholds, knots and centers. A representation_leaf() helper now returns the last step of the union's representation branch, found as the feature lineage finds it, and the summary and the verbose=3 log use it. Blocks ending in scikit-learn steps still get no summary entry, and the fingerprint does not hash the summary, so it does not change. Follow-up to #62

A wrapped Preprocessor refits only its target-aware blocks on each fold, and it recognised them from each block's RepresentationSpec or a fixed list of scikit-learn step names. A class registered with register_representation(..., supervision="supervised" | "optional") that has no RepresentationSpec, such as scikit-learn's TargetEncoder, was therefore taken to ignore y, and CrossFittedTransformer encoded every out-of-fold row with the all-data fit that had seen the row's own target: on a pure-noise target the out-of-fold encoding correlated with y at about 0.5. Such a block is now resolved from its registry entry: a "supervised" class, or an "optional" one with target_aware set, counts as target-aware, so it is refit on every fold and the lineage reports uses_target. Follow-up to #79

- A list of rows passed to transform after a DataFrame fit is matched by position (#94). - The fold-column check of cross-fitting is tested with deterministic folds instead of a frequency tie that a future scikit-learn could break differently (#90). - The unsupervised B/M/I splines never read y, even a y that cannot be aligned with X (#88). - The pipeline set_output test moves out of the polars-only module, so its pandas case also runs where polars is not installed (#94). - An optional registered class that defaults to target_aware=False follows the Preprocessor's target_aware and is then refit on every fold (#96 with the cross-fitting fix from #86).

ChrisW09 added 14 commits October 9, 2026 14:55

chore: typecheck correction for the new regression tests

1435b2b

chore: typecheck correction for the strict-JSON serialization test

b8c99b6

This was referenced Oct 9, 2026

CrossFittedTransformer(Preprocessor(add_missing_indicator=True)) crashes when a fold's training rows contain no missing value #89

Open

fix: resolve 10 bugs from a second library audit (#87-#96) #97

Open

Navigation Menu

Repository navigation

fix: resolve 10 high-priority bugs found in a library audit (#76-#85) - #86

fix: resolve 10 high-priority bugs found in a library audit (#76-#85)#86
ChrisW09 wants to merge 15 commits into
fix/open-bug-batchfrom
fix/bug-hunt-2

ChrisW09 commented Oct 9, 2026 •

edited

Loading

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ChrisW09 commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How the bugs were selected

Fixes

Follow-up commits: regressions found by the next audit

Behaviour changes to review

Testing

Not included (confirmed, lower priority, or design changes)

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ChrisW09 commented Oct 9, 2026 •

edited

Loading