Skip to content

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

Open
ChrisW09 wants to merge 17 commits into
fix/bug-hunt-2from
fix/bug-hunt-3
Open

ChrisW09 wants to merge 17 commits into
fix/bug-hunt-2from
fix/bug-hunt-3

Conversation

@ChrisW09

@ChrisW09 ChrisW09 commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

This PR fixes 10 more bugs from a second audit of the library. It is stacked on #86 and targets fix/bug-hunt-2, so this diff shows only the new commits. After #86 is merged, change the base to main.

The audit and its reviews also found three regressions that #75 and #86 introduce:

  • get_representation_spec() raises for every numerical transformer fitted on a DataFrame;
  • representation summaries are empty for missing-indicator blocks;
  • cross-fitting a Preprocessor that uses a registered target encoder leaks the target.

They are not on main, so they are fixed in follow-up commits on #86 itself; see #86's Follow-up commits section. #86's branch is merged into this one, so this branch is tested with those fixes and the two PRs merge without conflicts.

How the bugs were selected

This audit was sliced differently from the one behind #86. It covered six areas:

  • the Preprocessor itself (preprocessor.py, missing values, column detection)
  • the composition layer (factory, registry, output, inspection, search, serialization)
  • categorical encodings, embeddings and the extension protocol
  • numerical encodings, target-aware placement and cross-fitting
  • splines, feature maps and kernel approximations, checked against reference implementations
  • core infrastructure and scikit-learn API conformance (check_estimator on every exported transformer)

That produced about 40 new candidates. Everything already reported in #53–#85, or listed as deferred in #75 and #86, was excluded. Each of the 10 selected bugs:

Candidates were ranked by impact × likelihood, as in #86. Several were found independently by more than one area: the spline NaN rows by four, the empty column by three, the indicator cross-fitting crash and the unvalidated task by two each.

After the fixes were written, two independent reviews of the combined diff found four more problems in them. Each fix is folded into the commit it belongs to, either here or in #86's follow-up commits (details under Review follow-ups).

Fixes

# Issue Fix
1 #87 B/M/I splines turned a missing value into an all-zero row. For the I-spline that row equals the training minimum. The shared transform evaluates only observed rows; missing rows stay NaN (an optional bias column stays 1), as in the P-spline, natural-cubic, cubic-regression and tensor-product splines and the feature maps. Finite rows are bit-for-bit unchanged under every out-of-range policy. The two-feature thin-plate kernel had the same defect and now keeps a missing coordinate as NaN.
2 #95 A column with no observed value at fit crashed fit (numerical, one-hot) or vanished from the output while the metadata still listed it (int). The imputers keep such a column (keep_empty_features=True), so every feature keeps its block. fit warns, naming the column, and the constant policy treats it as constant. A method that cannot fit a constant column (splines, Box-Cox) raises a PretabDataError naming the column, but only when the same block fits observed values with the same y.
3 #88 B/M/I splines crashed on a 2-D y even when unsupervised. The unsupervised path never reads y. Target-aware placement, which fits one model on one target, slices y by rows and raises a clear IncompatibleParamsError for a multi-output y (pointing to target_aware=False) instead of an unrelated IndexError or broadcasting error. That covers the Preprocessor defaults and the regression presets, which are target-aware.
4 #89 CrossFittedTransformer(Preprocessor(add_missing_indicator=True)) crashed when a fold's training rows had no missing value. Each fold refits only the representation branch of an indicator block and keeps the all-data indicator. The width-mismatch message now also names category and indicator columns learned from the data, and asks for the same columns on every fold instead of only suggesting to disable adaptive sizing.
5 #96 Representations registered with supervision="optional" never received the Preprocessor's target_aware, so target_aware=False still fit on y. target_aware is injected for every optional spec, and placement_strategy only for specs that declare strategies, so built-ins are unchanged. Registering an optional class without a target_aware parameter raises TypeError.
6 #91 RepresentationSearchCV could not use group splitters and took no groups or fit params. fit(X, y, *, groups=None, **fit_params): groups go to cv.split, fit params are subset per fold (_check_method_params) and passed to the estimator and the refit. The Preprocessor and the scorer do not receive them; this is documented.
7 #92 RepresentationSearchCV around a classifier was not a classifier, so nested roc_auc CV gave NaN. predict_proba / predict_log_proba / decision_function are delegated with available_if. classes_, n_features_in_ and feature_names_in_ are exposed. The tags (and so is_classifier and stratified outer CV) follow the wrapped estimator.
8 #90 CrossFittedTransformer.fit_transform forced a dense float64 output. Fold outputs are stacked in their own container: sparse in the transform's format, pandas / polars frames by row order, dense with the folds' common dtype. A frame fold whose columns differ from the all-data fit raises instead of being misaligned.
9 #94 The Preprocessor rejected polars input, so set_output(transform="polars") pipelines crashed. Polars frames are converted column by column (no pyarrow needed; polars is never imported). RepresentationSearchCV and CrossFittedTransformer convert a polars frame once before taking fold rows, so every fold sees the dtypes of the whole frame, as with pandas input. A list of rows is read like an array; a pandas Series raises a clear error.
10 #93 An invalid task such as "Regression" was silently treated as classification. A shared validate_task is used by every fit that takes a task, by the Preprocessor before preset resolution (and by PreprocessorConfig), and by the selectors, which now dispatch explicitly on "classification". The error messages are unified.

Review follow-ups (folded into the commits they belong to)

Two independent reviews of the combined diff found these problems; each one now has a regression test.

Second review round (new commits on top)

After the PR was opened, four more independent reviews ran: an adversarial correctness review, a check for sibling code paths with the same root causes, a test-quality review (parent-commit checks and about 50 mutations), and a fact-check of the issues and PR texts. They led to these commits:

Commit What it fixes
Merge of fix/bug-hunt-2 Brings #86's new fix(cross-fitting): refit registered target-using classes on every fold into this branch. A class registered with supervision="supervised" / "optional" and no RepresentationSpec (such as scikit-learn's TargetEncoder) was treated as target-free and never refit per fold, so its out-of-fold encoding correlated with a pure-noise target at about 0.5. The #96 fix widened this, because optional classes now receive target_aware=True.
fix(search): convert a polars frame once before taking fold rows (#94) A polars integer column with a null converts to float as a whole but to int in a subset without the null, so one search fold detected it as categorical while the refit treated it as numerical. The search and cross-fitting now convert once, as for pandas input.
fix(splines): expand a missing value to a NaN row in the two-feature thin-plate basis (#87) np.where(r > 0, ..., 0.0) turned a missing coordinate into an all-zero kernel row: the same root cause as #87.
fix(placement): reject a multi-output target in target-aware placement with a clear error (#88) The #88 fix covered only the unsupervised path; the Preprocessor's target-aware defaults still failed on a 2-D y with an unrelated error.
docs: list the methods that cannot fit an all-missing column without imputation (#95) The note listed only the splines and Box-Cox, which is true only with imputation enabled.
test: pin the remaining review cases of the new fixes Adds tests for the four cases no test protected yet. One fold-column test now uses deterministic folds instead of a frequency tie. The pipeline test's pandas case now also runs where polars is absent. A last test pins the #86 leak fix together with #96's injection.

The fact-check also corrected wording in issues #87–#95 (scope, impact estimates and two permalink descriptions). Every reproduction still matched its issue byte for byte.

Behaviour changes to review

  • Missing values in B/M/I splines (missing_policy="propagate", numerical_imputation=None, standalone): these rows are now NaN instead of zeros, so a downstream model that rejects NaN will raise where it used to train silently on zero rows.
  • Empty columns:
    • they now fit and warn, except with a method that cannot fit a constant column (the splines, Box-Cox), which raises a PretabDataError naming the column;
    • an empty categorical column is filled with 0 by scikit-learn, so it becomes one int code, or a single one-hot column cat_<col>_0.
  • Fingerprints: the imputers are now built with keep_empty_features=True, so fingerprint_ changes for every newly fitted preprocessor with an imputer, although the output for non-empty columns is unchanged. Specs written before still load and reproduce exactly.
  • task:
    • invalid values raise at fit, also for methods that ignore the task;
    • Preprocessor(task=None) is now rejected. It used to resolve inconsistently: presets treated it as classification, the splines as regression, and PLE raised. The standalone splines still accept None as "regression".
    • The PLE, feature-map and CrossFittedTransformer error messages changed wording, but not type.
  • Custom optional representations:
    • they always receive the Preprocessor's target_aware, whose default is True;
    • a class without that parameter is rejected at registration;
    • an entry-point plugin without it is skipped with a ConfigWarning.
  • CrossFittedTransformer.fit_transform:
    • returns the same container and dtype as transform (sparse, float32, DataFrame, polars). For example, a standalone PLE now gives float32;
    • a fold whose frame columns differ from the all-data fit raises IncompatibleParamsError.
  • RepresentationSearchCV:
    • is_classifier(search) follows the estimator, so an outer cross_val_score with an int cv is now stratified for classifiers;
    • passing groups with a splitter that ignores them shows scikit-learn's usual warning.
  • Input types: a list of rows is accepted like an array; a pandas Series raises PretabDataError with a fix hint.
  • The golden regression baselines pass unchanged.

Testing

  • Full suite: 1944 passed, 59 skipped, 7 xfailed. fix: resolve 10 high-priority bugs found in a library audit (#76-#85) #86's head (with its follow-up commits) gives 1726 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, polars 2.0.0.
  • Every commit on its own passes the full suite, ruff check and ruff format --check, so the branch bisects cleanly.
  • Minimum dependencies (CI job: Python 3.10, numpy 1.24.4, pandas 2.0.3, scipy 1.10.1, scikit-learn 1.6.0, no polars / lightgbm): 1908 passed, 87 skipped, 7 xfailed, and scripts/quickstart.py passes.
  • Coverage: 94.59% locally with polars and lightgbm installed; the CI coverage job, which installs neither, reports about 93% (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, and the CI type check (numpy 2.2.6, scipy 1.15.3) passes.
  • CI: the full workflow (gh workflow run ci.yml --ref fix/bug-hunt-3) passes, including the 3.10–3.13 matrix on Linux, macOS and Windows, the minimum-dependency job and the optional-dependency jobs.
  • Reproductions: the script in each of the 10 issues was re-run on this branch and now shows the fixed behaviour. In RepresentationSearchCV cannot use group-based CV splitters and accepts no groups or fit parameters #91's script the sample_weight case still raises, because it uses GroupKFold without groups; it fits once groups is passed as well.
  • Legacy objects: a spec and a pickle written by 1.0.0 with add_missing_indicator / impute_with_indicator load, transform, and give to_spec, fingerprint_ and reproducibility_report on this branch.

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

  • Design questions:
    • P-spline and tensor-product penalties are index differences on a clamped knot vector, so a straight line still has a non-zero second-order penalty and λ→∞ fits are biased at the ends. A real Eilers–Marx fix changes every P-spline basis.
    • The adaptive window is ignored on the unsupervised path.
    • Fourier ignores output_dim in the Preprocessor.
    • Preprocessor(policy={"out_of_range": ...}) is accepted but not applied to the spline steps.
  • Lower-priority confirmed bugs:
    • LightGBM placement crashes with a RandomState instance.
    • LanguageEmbeddingTransformer calls encode(..., convert_to_numpy=True) on custom backends.
    • check_representation crashes on categorical input.
    • Registry names can collide with built-ins after normalization.
    • The default "int" encoder crashes on mixed int/str columns.
    • PeriodicEncodingTransformer does not validate period.
    • ThinPlateSplineTransformer declares allow_nan but cannot fit NaN.
    • feature_preprocessing={"col": None} crashes.
    • NumPy integer scalars (and np.float32) are rejected for cat_cutoff; np.float64 works because it subclasses float.
    • A target-aware spline with a y of the wrong length fails with an IndexError.
    • register_representation(..., preprocessor_compatible=False) is ignored for categorical methods.
    • ToFloatTransformer (standalone) crashes on polars and list input.
    • The deprecated OneHotFromOrdinalTransformer casts NaN to an integer code instead of raising.
    • Polars temporal columns lose their time zone and, like pandas datetimes, are not supported by the Preprocessor.
    • CrossFittedTransformer reports a dok_matrix fold output as "a dict of blocks".
  • Gaps left open here:
    • a column that is empty only inside one cross-fitting fold gets the unnamed constant-column error;
    • the CrossFittedTransformer spec of a Preprocessor fitted on an array names its inputs x0…, while the Preprocessor's own output names use feature_0…;
    • RepresentationSearchCV documents y as 1-D and still flattens a multi-output y.

Closes #87, closes #88, closes #89, closes #90, closes #91, closes #92, closes #93, closes #94, closes #95, closes #96

… basis

The B-, M- and I-spline transformers turned a NaN input into a finite all-zero
basis row (np.nan_to_num in each design matrix, a leftover from the old
extrapolate=False evaluation). For the I-spline that row is exactly the encoding
of the training minimum, so "missing" and "minimum" became indistinguishable;
for the B/M-spline it is impossible data. This was reachable standalone and from
the Preprocessor with missing_policy="propagate" or numerical_imputation=None,
while every other spline family and feature map propagates NaN.

The shared transform now evaluates only the observed rows and leaves the rows of
missing values NaN in that feature's basis block (an optional bias column stays
1). Finite rows are unchanged bit for bit under every out_of_range policy, and a
missing value does not trigger the "warn" / "error" policies. The Preprocessor
numerical_imputation docstring no longer claims that every numerical method
raises on missing values.

Fixes #87
The B-, M- and I-spline transformers flattened y and masked it with each
feature's missing-value mask even when no target-aware selector used it, so a
multi-output target of n rows (2n values after ravel) raised an IndexError in
fit. make_pipeline(BSplineTransformer(), Ridge()).fit(X, Y) with a 2-D Y failed,
as did the Preprocessor with numerical_method="bspline" / "mspline" / "ispline"
and target_aware=False, while scikit-learn's SplineTransformer and the other
spline families and feature maps ignore y there.

y is now read only when the target-aware selector places the knots; the
unsupervised and explicit knot_locations paths ignore it. On the selector path
y is sliced by rows like X instead of being flattened, matching the other
target-aware families; knots are unchanged for 1-D and column-vector targets.

Fixes #88
…across folds

CrossFittedTransformer(Preprocessor(add_missing_indicator=True)) and
missing_policy="impute_with_indicator" raised a width-mismatch error whenever a
fold's training rows held no missing value for a target-aware column with
missing values (always for a column with a single NaN). Each fold refit the
whole per-column block, including its MissingIndicator, which only emits a
column for features that had missing values at fit.

A target-aware block that is a representation + missing union now keeps its
all-data-fitted missing branch and refits only the representation branch on
the fold's training rows, so the out-of-fold features have the columns of
transform and out-of-fold rows still never see their own target. The
width-mismatch message now describes the general cause instead of adaptive
sizing only.

Fixes #89
…fit_transform

CrossFittedTransformer.fit_transform wrote the out-of-fold rows into a float64
buffer and densified sparse fold output, so it disagreed with transform: a
wrapped Preprocessor(categorical_method=None) raised on its unchanged string
columns, output_format="sparse" gave a dense array (bypassing the memory the
sparse format saves) where transform gave CSR, dtype=np.float32 gave float64,
and a DataFrame-returning estimator gave an ndarray.

The fold outputs are now collected and stacked in the original row order with
their own container: sparse blocks are stacked with scipy.sparse.vstack in the
transform format, pandas / polars DataFrames are concatenated (keeping the input
index when the blocks carry it, renumbered otherwise), and dense blocks take
their common dtype. The width check still runs on every fold, a DataFrame fold
whose columns differ from the all-data fit raises instead of being misaligned by
label, and dict output still raises.

Fixes #90
…imator

RepresentationSearchCV.fit took only (X, y) and called cv.split without groups,
so a group splitter (GroupKFold, LeaveOneGroupOut, GroupShuffleSplit,
StratifiedGroupKFold) always raised "The 'groups' parameter should not be None"
and fit(X, y, groups=g) raised a TypeError. Grouped data such as several rows
per patient could not be searched with a leakage-safe group split, and there
was no way to pass sample_weight or other fit parameters.

fit now takes a keyword-only groups, forwarded to cv.split, and **fit_params,
forwarded to the estimator's fit on every fold (per-sample values restricted to
the fold's training rows, as in scikit-learn's searches) and on the final
refit. The Preprocessor and the scorer do not receive them. Splits without
groups are unchanged.

Fixes #91
…fier

RepresentationSearchCV subclassed plain BaseEstimator and offered only predict
and score, so a search over a classifier had no classes_, predict_proba,
predict_log_proba or decision_function and is_classifier() was False. Nested
evaluation therefore broke: cross_val_score(search, X, y, scoring="roc_auc")
returned nan for every fold (with only a warning), and the outer split was a
plain KFold instead of a StratifiedKFold.

__sklearn_tags__ now takes the estimator type and classifier / regressor tags
of the wrapped estimator, as scikit-learn's search estimators do.
predict_proba, predict_log_proba and decision_function transform with
best_preprocessor_ and delegate to best_estimator_; they are available only
when the estimator provides them (judged from the estimator before fit and
from best_estimator_ after). classes_ is exposed from best_estimator_, and fit
records n_features_in_ / feature_names_in_ from best_preprocessor_. Regression
usage is unchanged.

Fixes #92
…or classification

The knot splines, the CART / LightGBM location selectors and the Preprocessor
config never checked task, and the selectors treated every value other than
"regression" as classification. A typo such as task="Regression" therefore gave
classification-tree knots, failed deep inside scikit-learn on a float target
("Unknown label type: continuous"), fit a continuous target as a many-class
LightGBM problem, and made every preset resolve numerical_method to "ple".

A shared validate_task helper in pretab.core.parameters now checks task at fit
in the B/M/I, natural cubic and cubic regression splines (None still means
"regression"), PLE, the feature maps, CrossFittedTransformer, the location
selectors and PreprocessorConfig.from_params. Preprocessor validates it before
preset resolution, so fit and get_resolved_config raise for any method or
preset; the selectors dispatch explicitly on "classification".

All of them raise InvalidParamError with one message
("<Estimator>.task = 'Regression' is invalid. ..."), replacing the differing
PLE and feature-map wording.

Fixes #93
to_dataframe treated every input that was not a dict or NumPy array as a pandas
frame, so Preprocessor.fit / transform on a polars DataFrame failed with
"AttributeError: 'list' object has no attribute 'duplicated'". A scikit-learn
Pipeline with set_output(transform="polars") therefore crashed as soon as it
handed its polars output to the Preprocessor.

A polars frame (detected via sys.modules, so polars is never imported) is now
converted to pandas column by column with Series.to_numpy, which needs no
pyarrow: names, order and numeric / boolean / temporal dtypes are kept, string,
categorical and enum columns become object columns, and nulls become NaN so
imputation and missing_policy see them as missing. Column-type detection,
get_feature_names_out, label matching at transform and polars output work as for
pandas input. RepresentationSearchCV and CrossFittedTransformer pass a polars
frame through like a pandas frame, so each fold sees the same column types as
the final fit.

Input without column labels is now read with np.asarray, so a list of rows works
like an array (positional, no feature_names_in_), while a pandas Series or 1D
input raises a PretabDataError instead of an AttributeError.

Fixes #94
… fit

A column missing on every fit row (e.g. an optional field that is empty in a
training window or CV fold) was dropped by the SimpleImputer, scikit-learn's
default for such a column. The rest of its pipeline then got no input: numerical
methods and one-hot failed with "Found array with 0 feature(s)", while "int" and
"none" silently removed the column from the output, output_dims_, blocks and
lineage, so get_feature_info() and verbose=2 fits raised an IndexError.

The imputers now keep such a column (keep_empty_features=True) and fill it with
0, or fill_value for strategy="constant", so every feature keeps its block and
later data where it is observed transforms with the fitted width. fit emits a
DataWarning naming the column, the constant policy treats an empty numerical
column as constant, and a method that cannot be fitted on a constant column
(splines, Box-Cox) raises a PretabDataError naming it. get_feature_info no
longer raises on a 0-width int block.

Fixes #95
The Preprocessor injected target_aware only into methods that declare placement
strategies, and register_representation defaults placement_strategies to ().
A representation registered with supervision="optional" therefore kept its own
target_aware default while y was still forwarded to it: with target_aware=False
it could fit on y (target leakage despite the "y-free fit" contract), and with
target_aware=True a class defaulting to False ignored it and lineage reported
uses_target=False.

target_aware is now passed to every optional method, and placement_strategy only
to methods that declare strategies, so built-ins are unchanged. As the docs say
an optional representation uses y only when target_aware=True,
register_representation now rejects an optional class without a target_aware
constructor parameter with a TypeError instead of letting it see y unchecked.

Fixes #96
RepresentationSearchCV and the clone-and-refit path of CrossFittedTransformer
took each fold's rows from the polars frame and let every subset be converted
to pandas on its own. A polars integer column with a null converts to float as
a whole frame but to int in a subset without the null, so the fold that missed
the null detected the column as categorical while the other folds and the
refit treated it as numerical: the search scored a different model than it
refit, and cross-fitting emitted raw codes for one fold.

Both now convert a polars frame to pandas once, before splitting, exactly as a
pandas frame is used, and the conversion moves to core so the cross-fitting
wrapper can share it. The docs now name the entry points that accept polars
input and that temporal columns are not supported.

Fixes #94
…thin-plate basis

ThinPlateSplineTransformer evaluates its two-feature kernel as
np.where(r > 0, r**2 * log(r), 0.0). A missing coordinate gives a NaN distance,
which fails r > 0 and so took the zero branch: the row became an all-zero
kernel row, the same silent fake data the B/M/I splines produced for a missing
value. One and three or more features already propagated NaN.

The zero branch is now selected by r == 0, so a NaN distance stays NaN and the
row is NaN like in every other spline family. Finite rows are unchanged.

Fixes #87
…t with a clear error

Target-aware placement fits one decision tree or boosting model on one target,
but the shared location selector and PLE flattened y. A multi-output target of
n rows became 2n values and failed with an unrelated IndexError or NumPy
broadcasting error; this is what the Preprocessor's default (target-aware)
settings and the regression presets hit with a 2-D y, after the unsupervised
path was fixed to ignore y.

A shared single_target() check now flattens a column vector as before and
raises an IncompatibleParamsError for more than one output column, pointing to
target_aware=False.

Fixes #88
…imputation

The edge-case note said only the splines and Box-Cox reject a column with no
observed value at fit. That holds with imputation; with imputation disabled the
column stays missing, and every method except the scalers, "quantile" and
"none" raises the typed error naming it.

Refs #95
- 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).

This branch has not been deployed

No deployments
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