google-research / google-research/tabfm
Estimators define no _parameter_constraints; all 13 numeric parameters accept 0 and negative values
- 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
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