Skip to content

Conversation

@priorphil
Copy link
Contributor

No description provided.

@priorphil priorphil requested a review from bejaeger October 31, 2025 11:13
@priorphil priorphil requested a review from a team as a code owner October 31, 2025 11:13
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 adds kditransform as a project dependency. The change is straightforward, but I've left a comment regarding sorting dependencies for better maintainability.

Additionally, with kditransform becoming a required dependency, the fallback logic in src/tabpfn/preprocessors/kdi_transformer.py (which handles cases where kditransform is not installed) appears to be redundant. You may want to consider removing the try-except block around the import in a future pull request to simplify the code.

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".

Copy link
Contributor

@bejaeger bejaeger left a comment

Choose a reason for hiding this comment

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

Thanks!

@priorphil priorphil merged commit ddc5fc9 into main Oct 31, 2025
10 checks passed
@priorphil priorphil deleted the phil/add_kdi_dep branch October 31, 2025 12:57
@tankylz
Copy link

tankylz commented Nov 3, 2025

Hello there, I want to check the purpose of the new dependency kditransform. This is because I was using the older versions which didn't have this as a required dependency. Reinstalling the packages now lead to dependencies conflict with other packages. Thanks in advanced for the clarifications!

@priorphil
Copy link
Contributor Author

We found that using the KDI transform for some pre-processors improves predictive performance.
Could you give more details about the package conflict? Do you have other deps that depend on a different version of the KDI transform, do the KDI dependencies cause the mismatch, or something else?

@tankylz
Copy link

tankylz commented Nov 3, 2025

The conflict comes from sklearn actually. We are using tabpfgen as well (which I understand is from a different group).

Running on Linux:

tabpfn 2.2.1 depends on scikit-learn<1.8 and >=1.2.0
tabpfgen 0.1.4 depends on scikit-learn==1.5.2
kditransform 1.2.0 depends on scikit-learn>=1.6.0

oscarkey pushed a commit that referenced this pull request Nov 12, 2025
* Record copied public PR 580

* Add kdi transform to deps. (#580)

(cherry picked from commit ddc5fc9)

---------

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