Skip to content

Conversation

@magnus-priorlabs
Copy link
Contributor

…e to better Performance

Changed back to vanilla ColumnTransformer (despite incorrect categorical column indicators to feature mappings afterwards), due to improved performance. Added explanatory comment.

Issue

Please link the corresponding GitHub issue. If an issue does not already exist,
please open one to describe the bug or feature request before creating a pull request.

This allows us to discuss the proposal and helps avoid unnecessary work.
Originally "solved" in this PR (#539).

Motivation and Context


Public API Changes

  • No Public API changes
  • Yes, Public API changes (Details below)

How Has This Been Tested?

locally

Checklist

  • The changes have been tested locally.
  • Documentation has been updated (if the public API or usage changes).
  • A entry has been added to CHANGELOG.md (if relevant for users).
  • The code follows the project's style guidelines.
  • I have considered the impact of these changes on the public API.

…e to better Performance

Changed back to vanilla ColumnTransformer (despite incorrect categorical column indicators to feature mappings afterwards), due to improved performance.
Added explanatory comment
@magnus-priorlabs magnus-priorlabs requested a review from a team as a code owner November 5, 2025 09:12
@magnus-priorlabs magnus-priorlabs requested review from oscarkey and removed request for a team November 5, 2025 09:12
@CLAassistant
Copy link

CLAassistant commented Nov 5, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request reverts the use of OrderPreservingColumnTransformer back to scikit-learn's ColumnTransformer to improve performance. The author has correctly identified and documented that this change can lead to misaligned categorical feature indices. My feedback focuses on making this known issue more visible as technical debt by adding a TODO comment.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@bejaeger bejaeger merged commit edb3589 into PriorLabs:main Nov 5, 2025
9 of 10 checks passed
oscarkey pushed a commit that referenced this pull request Nov 12, 2025
…olumnTransformer (#233)

* Record copied public PR 596

* Changed OrderPreservingColumnTransformer back to ColumnTransformer (#596)

(cherry picked from commit edb3589)

---------

Co-authored-by: mirror-bot <mirror-bot@users.noreply.github.com>
Co-authored-by: magnus-priorlabs <magnus@priorlabs.ai>
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.

4 participants