Skip to content

Remove sklearn<=1.6 compatibility code - #985

Merged
solegalli merged 1 commit into
narwhals-migrationfrom
remove-sklearn16-compat
Jul 31, 2026
Merged

Remove sklearn<=1.6 compatibility code#985
solegalli merged 1 commit into
narwhals-migrationfrom
remove-sklearn16-compat

Conversation

@solegalli

Copy link
Copy Markdown
Collaborator

Summary

The project already requires scikit-learn>=1.7.0 (pyproject.toml, tox.ini, .circleci/config.yml all already only test 1.7.1), so every if sklearn_version </> parse_version("1.6") branch across the codebase was dead code — the "older sklearn" side could never execute. This PR removes that dead code, keeping only the always-taken >=1.6 branch (the one using check_estimator(expected_failed_checks=...)).

Changes

  • feature_engine/tags.py — collapsed the version check in _return_tags(), the shared helper used by ~20 estimator classes.
  • 11 tests/**/test_check_estimator_*.py files — collapsed each if/else on the sklearn 1.6 cutoff, dropped now-unused sklearn/parse_version imports.
  • tests/test_wrappers/test_sklearn_wrapper.py — also collapsed the _OneHotEncoder() test helper's sparse/sparse_output branch (sklearn<1.2, dead for the same reason). Left the separate KBinsDiscretizer(quantile_method=...) branch (sklearn<1.7) untouched — different threshold, intentionally out of scope here.
  • tests/test_creation/test_geo_features.py__sklearn_tags__ always exists at sklearn>=1.7, so dropped the hasattr() guard.
  • tests/check_estimators_with_parametrize_tests.py — deleted entirely. A standalone, non-CI reference file documenting the pre-1.6 parametrize_with_checks() call signature.

Not touched, on purpose: every _more_tags()/__sklearn_tags__() method definition across feature_engine/. _more_tags() is feature_engine's own internal metadata/xfail-checks store (read directly by tests/estimator_checks/*.py), not a legacy sklearn shim — it has no equivalent in the new tags dataclass. __sklearn_tags__() is the current, correct API.

Known pre-existing gap (not introduced here)

tests/test_prediction/test_check_estimator_prediction.py only ever had the dead sklearn<1.6 branch — no >=1.6 version of test_check_estimator_from_sklearn was ever written for the prediction module (its own # TODO already flagged this). Removing the dead branch leaves that module with zero test_check_estimator_from_sklearn coverage. This already existed before this PR (the branch just wasn't reachable); flagging it here as a follow-up rather than expanding scope to write the missing test.

Test plan

  • pytest tests/ — identical result before and after this change: 2010 passed, 114 failed (all 114 are pre-existing narwhals-migration WIP failures, unrelated — confirmed by diffing the exact failing test names before/after, not just counts)
  • flake8 feature_engine tests — clean
  • mypy feature_engine — the one remaining error (datetime/datetime_subtraction.py) is pre-existing and unrelated, confirmed present on the branch before this PR too
  • grep -rn 'parse_version("1.6")' across feature_engine/ and tests/ — zero hits

The project already requires scikit-learn>=1.7.0 (pyproject.toml,
tox.ini, .circleci/config.yml), so the sklearn<=1.6 branches of every
check_estimator/tags conditional were dead code. This removes them,
keeping only the >=1.6 branch (the one using
check_estimator(expected_failed_checks=...)):

- feature_engine/tags.py: collapse the sklearn_version > 1.6 check in
  _return_tags(), the shared helper used across ~20 estimator classes.
- 11 tests/**/test_check_estimator_*.py files: collapse each
  if/else on sklearn_version vs 1.6, drop the now-unused sklearn/
  parse_version imports and sklearn_version variables.
- tests/test_prediction/test_check_estimator_prediction.py: this file
  had no >=1.6 branch, only the dead <1.6 one (its own TODO already
  flagged this). Removing it leaves the prediction module with no
  test_check_estimator_from_sklearn coverage - a pre-existing gap,
  not introduced by this change, left as a follow-up.
- tests/test_creation/test_geo_features.py: __sklearn_tags__ always
  exists at sklearn>=1.7, so drop the hasattr() guard around it.
- tests/test_wrappers/test_sklearn_wrapper.py: also collapse the
  _OneHotEncoder() test helper's sparse/sparse_output branch (sklearn
  <1.2 compat, dead for the same reason). The separate
  KBinsDiscretizer(quantile_method=...) branch (sklearn<1.7) is
  intentionally left as-is - different threshold, out of scope here.
- tests/check_estimators_with_parametrize_tests.py: delete entirely.
  A standalone, non-CI reference file documenting the pre-1.6
  parametrize_with_checks() call signature.

_more_tags()/__sklearn_tags__() method definitions are untouched:
_more_tags() is feature_engine's own internal metadata/xfail-checks
store (read by tests/estimator_checks/*.py), not a legacy sklearn
shim, and __sklearn_tags__() is the current sklearn API.

Verified: identical test suite pass/fail counts before and after
(2010 passed, 114 failed - all 114 are pre-existing narwhals-migration
WIP failures unrelated to this change), flake8 and mypy clean (the one
remaining mypy error is pre-existing in datetime_subtraction.py,
unrelated to this PR).
@solegalli
solegalli merged commit 91377ea into narwhals-migration Jul 31, 2026
3 of 9 checks passed
@solegalli
solegalli deleted the remove-sklearn16-compat branch July 31, 2026 08:51
solegalli added a commit that referenced this pull request Jul 31, 2026
The project already requires scikit-learn>=1.7.0 (pyproject.toml,
tox.ini, .circleci/config.yml), so the sklearn<=1.6 branches of every
check_estimator/tags conditional were dead code. This removes them,
keeping only the >=1.6 branch (the one using
check_estimator(expected_failed_checks=...)):

- feature_engine/tags.py: collapse the sklearn_version > 1.6 check in
  _return_tags(), the shared helper used across ~20 estimator classes.
- 11 tests/**/test_check_estimator_*.py files: collapse each
  if/else on sklearn_version vs 1.6, drop the now-unused sklearn/
  parse_version imports and sklearn_version variables.
- tests/test_prediction/test_check_estimator_prediction.py: this file
  had no >=1.6 branch, only the dead <1.6 one (its own TODO already
  flagged this). Removing it leaves the prediction module with no
  test_check_estimator_from_sklearn coverage - a pre-existing gap,
  not introduced by this change, left as a follow-up.
- tests/test_creation/test_geo_features.py: __sklearn_tags__ always
  exists at sklearn>=1.7, so drop the hasattr() guard around it.
- tests/test_wrappers/test_sklearn_wrapper.py: also collapse the
  _OneHotEncoder() test helper's sparse/sparse_output branch (sklearn
  <1.2 compat, dead for the same reason). The separate
  KBinsDiscretizer(quantile_method=...) branch (sklearn<1.7) is
  intentionally left as-is - different threshold, out of scope here.
- tests/check_estimators_with_parametrize_tests.py: delete entirely.
  A standalone, non-CI reference file documenting the pre-1.6
  parametrize_with_checks() call signature.

_more_tags()/__sklearn_tags__() method definitions are untouched:
_more_tags() is feature_engine's own internal metadata/xfail-checks
store (read by tests/estimator_checks/*.py), not a legacy sklearn
shim, and __sklearn_tags__() is the current sklearn API.

Verified: identical test suite pass/fail counts before and after
(2010 passed, 114 failed - all 114 are pre-existing narwhals-migration
WIP failures unrelated to this change), flake8 and mypy clean (the one
remaining mypy error is pre-existing in datetime_subtraction.py,
unrelated to this PR).
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