google-research / google-research/tabfm

Estimators define no _parameter_constraints; all 13 numeric parameters accept 0 and negative values

Open
#94 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
2.6k
Forks
270
Avg merge
1d 7h
Merged PRs (30d)
1

Description

Related to #92 but a different surface: that one is the `TabFM` model constructor, this is the scikit-learn estimator wrapper.

`TabFMClassifier` and `TabFMRegressor` subclass `BaseEstimator`, but do not define `_parameter_constraints`. Modern scikit-learn runs its parameter validation from that attribute, so without it **none** of the estimator's parameters is ever validated — not by tabfm, and not by sklearn either.

I tested every numeric parameter at `0` and `-1` on `b15593e4c1111ddb5f4f30dd2957df2edbaa04ca`. All 26 combinations are accepted:

```
n_estimators 0, -1 ACCEPTED
outlier_threshold 0, -1 ACCEPTED
max_num_features 0, -1 ACCEPTED
max_num_rows 0, -1 ACCEPTED
softmax_temperature 0, -1 ACCEPTED
batch_size 0, -1 ACCEPTED
num_folds_for_cv 0, -1 ACCEPTED
n_feature_crosses 0, -1 ACCEPTED
n_svd_features 0, -1 ACCEPTED
total_svd_pool 0, -1 ACCEPTED
nnls_beta 0, -1 ACCEPTED
calibration_lambda 0, -1 ACCEPTED
min_rows_for_single_val_split 0, -1 ACCEPTED
```

Some of these have consequences further in. `softmax_temperature` is #93 — a negative value silently inverts the prediction. `n_estimators` reaches `np.ones(n_est) / n_est` (lines 2649, 2651, 3624, 3626), so zero gives a divide. `num_folds_for_cv` drives the cross-validation split. `batch_size` becomes a stride.

Adding `_parameter_constraints` would be the idiomatic fix and would cover all of them at once, including #93. It also brings the estimators into line with what `check_estimator` expects, which may be worth having anyway since the class already advertises the sklearn interface.

I have not tried to enumerate what each bad value does downstream — several need a fitted model — so I am reporting the missing validation rather than claiming a specific failure for each.

Disclosure: I used an AI assistant to help find this. I ran the matrix myself.

Contributor guide

Open the contributing guide

Research direction

Start with the TabFMClassifier and TabFMRegressor estimator wrappers and inspect how their numeric parameters are declared. Add the missing _parameter_constraints with appropriate validation for the listed parameters, then run the invalid-value matrix and sklearn check_estimator; done means zero and negative inputs are rejected rather than accepted.

Written by the indexing model from the issue text.

Assessment

Tech stack
numpy, python, scikit-learn
Domain
machine-learning, testing-qa
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
64/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.