[MNT] narwhals migration - #965
Open
solegalli wants to merge 4 commits into
Open
Conversation
This was referenced Jul 26, 2026
Contributor
|
Hi @solegalli — I'd like to help with the narwhals migration. If it is still free, I can take Please let me know if that module is already spoken for — happy to pick another (e.g. a simpler preprocessing piece) instead. |
Collaborator
Author
|
That is actually a good one to start with. The tests should pass with pandas. I am not sure they will pass with polars because we need to change the functions that select variables, on which I am working on right now and will soon make a PR. |
Contributor
|
Started on scaling as discussed — opened a PR against this branch: will link here once created (see latest open PR from @ojassharma7 titled migrate scaling module to narwhals). Pandas tests for the module pass locally. As you said, polars may still need your variable-selection updates. |
2 tasks
Contributor
|
Scaling PR: #979 |
solegalli
force-pushed
the
narwhals-migration
branch
from
July 30, 2026 12:09
b2c6d4f to
c6dd5f3
Compare
* update dataframe checks * update dataframe checks take 2 * update dataframe checks take 3 * update docstrings * refactor dataframe checks * fix mypy error * add missing type hints * add missing matching error syntax * finalise tests for df checks'
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
force-pushed
the
narwhals-migration
branch
2 times, most recently
from
July 31, 2026 12:30
8fe8359 to
ea95750
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.