Migrate DecisionTreeDiscretiser to narwhals, add polars support - #1042
Open
solegalli wants to merge 1 commit into
Open
Migrate DecisionTreeDiscretiser to narwhals, add polars support#1042solegalli wants to merge 1 commit into
solegalli wants to merge 1 commit into
Conversation
DecisionTreeDiscretiser now accepts pandas or polars input via narwhals, extending BaseNumericalTransformer directly (independent of the BaseDiscretiser migration). Never imports pandas; confirmed the module loads with pandas import blocked. Merge (single narwhals codepath, one is_pandas branch only at the final column reassembly) over split (branching at every column-selection call site): benchmarked at 10k/50k/100k rows x 1/2/10 cols, full fit+transform time is dominated by GridSearchCV tree training (10-1500ms) vs plumbing (~0.05-1.4ms per call, <1% of total even where a hand-branched pandas path was ~2x faster on the isolated plumbing microbenchmark). Merge also avoids the one-column-at-a-time write pattern that caused pandas fragmentation warnings in the DecisionTreeFeatures migration. Added optional n_jobs (default None = sequential, unchanged behaviour), parallelizing the per-variable tree fits with joblib threads, mirroring DecisionTreeFeatures. Benchmarked: net loss on small workloads (2 vars, small grid: 0.6-0.8x), real win once there's enough work (2-50 vars with a larger grid: 1.4-2.3x). Verified n_jobs=2 produces identical trees and predictions to n_jobs=None. Bug found and fixed (introduced by the base-transformer narwhals migration, not present pre-migration): check_X used to always copy its pandas input; the narwhals-based check_X no longer does, so the old transform()'s in-place `X[feature] = ...` assignments would have mutated the caller's original dataframe. Rewrote transform() to batch every replacement column and apply them in one non-mutating `.assign()` (pandas) / `.with_columns()` (polars) call instead, which also sidesteps polars' immutability and avoids per-column pandas fragmentation. Reimplemented pandas.cut's binning (bin_number/boundaries outputs) without importing pandas: np.digitize for bin assignment, and a from-scratch port of pandas' internal `_round_frac`/`_infer_precision` label-rounding algorithm (rounds each edge, bumping precision globally if that would collide two edges) so boundary labels are byte-for-byte identical to the old pd.cut output. Verified against pandas.cut directly across 500 randomized threshold/precision/value trials with zero mismatches, in addition to the existing hardcoded-value tests passing unmodified. Tests rewritten to one parametrized test per behavior over make_df in [pd.DataFrame, pl.DataFrame], replacing the pandas-only df_normal_dist/df_discretise fixtures with local data dicts (matching the DecisionTreeFeatures precedent, since those shared fixtures are still pandas-only). Fixed test_non_fitted_error, which was instantiating EqualWidthDiscretiser instead of DecisionTreeDiscretiser (a pre-existing copy-paste bug, confirmed present on main before this migration). tests/test_discretisation full suite: 123 passed (was 108 pre-migration, +15 from parametrization), same 5 pre-existing check_estimator failures (numpy-array input rejected by narwhals check_X, unrelated to this file, confirmed identical on the pre-migration baseline). flake8 and mypy clean. sphinx -W build produces only the pre-existing linkcode_resolve warning (confirmed identical on baseline). Docs: added "With polars" and "Training trees in parallel" sections, verified against real output (network available this session, so the existing fetch_openml house-prices example was re-run and confirmed still accurate). The two `binner_dict_` boundary/bin_number code blocks now display floats as plain numbers as before; current numpy's list repr actually renders them as np.float64(...), a numpy-version-only cosmetic drift present across the whole docs tree and not caused by this migration, left as-is and noted here instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Migrates
DecisionTreeDiscretiserto narwhals with polars support.DecisionTreeDiscretisernow accepts pandas or polars input via narwhals, extendingBaseNumericalTransformerdirectly — independent of theBaseDiscretisermigration, so this branch is a single commit offnarwhals-migrationwith no base-branch stack. Never imports pandas; confirmed the module loads with pandas import blocked.Merge vs split: single narwhals codepath (one
is_pandasbranch only at final column reassembly). Benchmarked at 10k/50k/100k rows × 1/2/10 cols — full fit+transform time is dominated byGridSearchCVtree training (10–1500ms) vs plumbing (~0.05–1.4ms per call, <1% of total even where a hand-branched pandas path was ~2x faster on the isolated microbenchmark). Merge also avoids the one-column-at-a-time write pattern that caused pandas fragmentation warnings in theDecisionTreeFeaturesmigration.Added optional
n_jobs(defaultNone= sequential, unchanged behaviour), parallelising per-variable tree fits with joblib threads, mirroringDecisionTreeFeatures. Net loss on small workloads (0.6–0.8x), real win with enough work (1.4–2.3x).n_jobs=2produces identical trees and predictions ton_jobs=None.Bug fixed (introduced by the base-transformer narwhals migration, not present pre-migration):
check_Xused to always copy its pandas input; the narwhals-basedcheck_Xno longer does, so the oldtransform()'s in-placeX[feature] = ...assignments would have mutated the caller's original dataframe. Rewrotetransform()to batch every replacement column and apply them in one non-mutating.assign()(pandas) /.with_columns()(polars) call.Reimplemented
pandas.cut's binning (bin_number/boundariesoutputs) without importing pandas:np.digitizefor assignment, plus a from-scratch port of pandas' internal_round_frac/_infer_precisionlabel-rounding so boundary labels are byte-for-byte identical to oldpd.cutoutput (verified across 500 randomized trials, zero mismatches).Tests rewritten to one parametrized test per behaviour over
[pd.DataFrame, pl.DataFrame]. Fixedtest_non_fitted_error, which instantiatedEqualWidthDiscretiserinstead ofDecisionTreeDiscretiser(pre-existing copy-paste bug, confirmed on main).Verified:
tests/test_discretisation— 123 passed (was 108, +15 from parametrization), same 5 pre-existingcheck_estimatorfailures. flake8 / mypy clean, sphinx -W clean. "With polars" and "Training trees in parallel" doc sections added, verified against real output.