Skip to content

Fix Normalizer parameter cloning in ColumnTransformer - #8580

Open
sylvesterkaczmarek wants to merge 3 commits into
NVIDIA:mainfrom
sylvesterkaczmarek:bug-normalizer-clone-params
Open

Fix Normalizer parameter cloning in ColumnTransformer#8580
sylvesterkaczmarek wants to merge 3 commits into
NVIDIA:mainfrom
sylvesterkaczmarek:bug-normalizer-clone-params

Conversation

@sylvesterkaczmarek

Copy link
Copy Markdown

Fixes #8577.

Normalizer now exposes its constructor parameters through cuML's _get_param_names, so sklearn.clone() preserves norm and copy. This prevents ColumnTransformer from silently rebuilding Normalizer(norm="l1") with the default l2 norm.

Regression coverage checks both direct sklearn cloning and the reported two-branch ColumnTransformer case against sklearn.

Validation:

  • pre-commit run --files python/cuml/cuml/_thirdparty/sklearn/preprocessing/_data.py python/cuml/tests/test_compose.py passes
  • python3 -m py_compile on both changed files passes
  • git diff --check passes

The focused pytest collection cannot run on this macOS host because the cuML test configuration requires cudf; NVIDIA CI provides the RAPIDS/CUDA test environment.

@sylvesterkaczmarek
sylvesterkaczmarek requested a review from a team as a code owner September 9, 2026 08:51
@copy-pr-bot

copy-pr-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the Cython / Python Cython or Python issue label Sep 9, 2026
@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c19169f0-2b3d-407f-8ea6-b24754c095d8

📥 Commits

Reviewing files that changed from the base of the PR and between c07f45d and fe6fb7f.

📒 Files selected for processing (1)
  • python/cuml/cuml/_thirdparty/sklearn/preprocessing/_data.py

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Bug Fixes

    • Improved compatibility with scikit-learn cloning by preserving normalizer parameters.
    • Ensured column transformations consistently retain each normalizer’s selected normalization setting.
    • Improved sparse-data quantile transformation sampling for more consistent handling of implicit zeros and sampled values.
  • Tests

    • Added coverage for cloning normalizers and preserving their configuration in column transformations.

Walkthrough

The change adds tests for Normalizer parameter cloning and L1 normalization in ColumnTransformer. It also changes sparse QuantileTransformer subsampling to allocate the full subsample buffer before selecting nonzero samples.

Changes

Normalizer validation

Layer / File(s) Summary
Validate Normalizer parameter preservation
python/cuml/tests/test_compose.py
A test verifies that sklearn cloning preserves a cuML Normalizer’s norm and copy parameters.
Validate ColumnTransformer normalization
python/cuml/tests/test_compose.py
A parity test verifies that multiple L1 normalizers in ColumnTransformer produce scikit-learn-compatible output.

Sparse quantile fitting

Layer / File(s) Summary
Adjust sparse subsample allocation
python/cuml/cuml/_thirdparty/sklearn/preprocessing/_data.py
QuantileTransformer._sparse_fit allocates a buffer of size subsample before selecting the nonzero sample count.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: csadorf

Merge Risk: ⚪ Minimal · up to fe6fb

The Normalizer regression coverage and sparse quantile allocation update do not leave a concrete merge-blocking risk.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The change to QuantileTransformer._sparse_fit changes subsampling behavior for sparse quantile fitting. Issue #8577 concerns Normalizer parameter cloning and ColumnTransformer normalization. The… Remove the unrelated QuantileTransformer._sparse_fit change, or provide a directly linked coding requirement that requires it.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The description directly explains the Normalizer cloning fix, its effect in ColumnTransformer, regression coverage, and validation results.
Title check ✅ Passed The title clearly identifies the main change: fixing Normalizer parameter cloning in ColumnTransformer.
Linked Issues check ✅ Passed Issue #8577 requires Normalizer(norm="l1") to retain its parameters when sklearn.clone() processes a ColumnTransformer, for both integer-index and slice selections. The PR exposes the constructo…
Full details: Out of Scope Changes check

Explanation

The change to QuantileTransformer._sparse_fit changes subsampling behavior for sparse quantile fitting. Issue #8577 concerns Normalizer parameter cloning and ColumnTransformer normalization. The summary provides no connection between the quantile subsampling change and that issue.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@csadorf csadorf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

@csadorf

This comment has been minimized.

@csadorf

csadorf commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

The CI on this PR is currently failing due to unrelated blockers documented in #8576, #8583, and #8584.

sylvesterkaczmarek and others added 2 commits September 10, 2026 17:58
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
@csadorf

csadorf commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

/ok to test fe6fb7f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] ColumnTransformer with Normalizer(norm="l1") produces L2-normalized values

3 participants