Skip to content

Validate ensemble parameters before estimator fitting - #104

Open
ayoubchikri wants to merge 4 commits into
google-research:mainfrom
ayoubchikri:fix/estimator-parameter-validation
Open

ayoubchikri wants to merge 4 commits into
google-research:mainfrom
ayoubchikri:fix/estimator-parameter-validation

Conversation

@ayoubchikri

Copy link
Copy Markdown

Summary

Invalid ensemble settings currently reach preprocessing and can later fail with unrelated errors or produce invalid ensemble weights. For example, n_estimators=0 is accepted by fit, while nnls_beta=1.1 permits extrapolation outside the documented blend.

Add scikit-learn parameter constraints and validate them at the start of classifier/regressor fitting. Cover ensemble size, optional feature/row limits, batching, CV folds, SVD pool size, NNLS blending, the validation split threshold, and the classifier calibration penalty. This also validates values supplied through set_params.

Preserve the supported zero/None sentinels for batching and SVD, a zero validation-split threshold, and both endpoints of the NNLS blend.

Refs #94. This addresses the ensemble controls and calibration penalty; temperature and feature-cross validation are outside this patch.

Validation

  • Full local suite: 65 passed, 34 skipped (OMP_NUM_THREADS=2 MKL_NUM_THREADS=2 PYTHONPATH=. pytest -q). The skips are JAX-dependent tests; the available PyTorch tests pass.
  • The 28 invalid-configuration regression cases fail against upstream fbb6655 and pass with this patch.
  • Regression coverage checks that invalid settings fail before preprocessing and that valid boundary settings still fit for both estimators.
  • Python compilation, lint of the new test file, and git diff --check pass.

Environment: Python 3.12.12, scikit-learn 1.9.0, NumPy 1.26.4, PyTorch 2.2.2, macOS CPU.

@google-cla

google-cla Bot commented Sep 15, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new module-level import introduces a dependency floor that the package metadata does not declare. pyproject.toml still allows unconstrained scikit-learn with Python >=3.11, but scikit-learn 1.1.3 has Python 3.11 wheels and does not contain sklearn.utils._param_validation; import tabfm.src.classifier_and_regressor will therefore fail in an environment that satisfies TabFM's declared requirements. Please add an appropriate minimum scikit-learn version (the module exists from 1.2 onward), or avoid depending on this private sklearn API, and cover the supported dependency floor in CI.

@ayoubchikri

Copy link
Copy Markdown
Author

@sylvesterkaczmarek Thanks for catching this. I declared scikit-learn>=1.6 in pyproject.toml: the existing validate_data import also requires 1.6, so a 1.2 floor would still leave this module unimportable.

I added a Python 3.11 CI job that installs scikit-learn 1.6.0, checks both estimator imports, and runs the parameter-validation tests. The imports and all 41 targeted tests pass locally at that floor. The workflow security checks are green.

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed current 671dd4aa. The dependency contract now matches the implementation: scikit-learn>=1.6 is declared, which also covers the pre-existing validate_data import, and the new minimum-version CI job installs exactly 1.6.0, verifies the estimator imports, and runs the parameter-validation tests. My earlier blocker is resolved.

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.

2 participants