Add multi-output classifier support and refresh examples #207
Conversation
noahho
requested review from
bejaeger and
Copilot
and removed request for
a team
Summary of ChangesHello @noahho, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for multi-output regression and classification through new scikit-learn compatible wrappers, TabPFNMultiOutputRegressor and TabPFNMultiOutputClassifier. The changes include the core implementation, an illustrative example, and new tests. While the feature is a valuable addition, I've identified a critical issue in src/tabpfn_extensions/multioutput.py where the implementation of get_params is incompatible with scikit-learn's clone function, which would break its use in cross-validation or hyperparameter tuning. Additionally, the new tests in tests/test_multioutput.py do not cover this cloning functionality or the case where the estimator is created internally, which is why the bug was missed. My review includes detailed feedback on how to resolve the compatibility issue and improve test coverage.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull Request Overview
This pull request extends TabPFN's multi-output capabilities to support both regression and classification tasks through unified wrappers, while refreshing examples to demonstrate both use cases with missing value handling.
Key Changes:
- Introduced
TabPFNMultiOutputClassifieralongside the existing regressor functionality using a shared mixin pattern for initialization logic - Refactored examples to demonstrate both regression and classification workflows without focusing specifically on imputation
- Expanded test coverage to verify missing value handling works correctly for both regression and classification across different backends
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tabpfn_extensions/multioutput.py | Implements shared mixin class for multi-output wrappers supporting both regression and classification |
| src/tabpfn_extensions/init.py | Exports new multi-output classifier class alongside existing regressor |
| tests/test_multioutput.py | Adds comprehensive tests for both regression and classification with missing values across backends |
| tests/conftest.py | Adds sys.path configuration to ensure test imports work correctly |
| examples/multioutput/multioutput_prediction.py | Demonstrates both multi-output regression and classification with missing data |
| examples/multioutput/README.md | Documents the multi-output functionality for both regression and classification |
| examples/README.md | Updates directory listing to include multioutput examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ion-with-imputation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces multi-output classifier support by creating a shared mixin, which is a solid design choice. The new examples and tests are also well-structured. I've identified a high-severity bug in the set_params implementation that breaks scikit-learn compatibility and should be addressed. Additionally, I've provided a few medium-severity suggestions to enhance the maintainability of the test setup and improve the code quality within the new example script.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for multi-output regression and classification by wrapping scikit-learn's MultiOutputRegressor and MultiOutputClassifier. The implementation is clean and correctly integrates with scikit-learn's clone mechanism by handling tabpfn_params. The PR also adds a new example and tests. My review focuses on improving the examples and test coverage. I've pointed out that the example and tests don't actually demonstrate the handling of missing values, despite it being mentioned. More importantly, the new TabPFNMultiOutputClassifier is completely untested. I've provided suggestions to add a test for the classifier and to include missing values in both the example and the existing test case.
| assert np.isfinite(cloned_predictions).all() | ||
|
|
||
| cloned_score = r2_score(y, cloned_predictions, multioutput="uniform_average") | ||
| assert cloned_score > 0.2 |
There was a problem hiding this comment.
The current tests only cover TabPFNMultiOutputRegressor. To ensure the new TabPFNMultiOutputClassifier is also working as expected and is robust, please add a corresponding test case for it. This test should cover fitting, prediction, and cloning, similar to test_multioutput_regression. You can use sklearn.datasets.make_multilabel_classification to generate test data. The absence of tests for the classification wrapper is a significant gap.
| ) | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # 1. Multi-output regression with missing features |
There was a problem hiding this comment.
The comment here states "with missing features", but the synthetic data generated for X_reg does not contain any missing values. This is misleading for users looking at this example. To align the comment with the code, you could either remove "with missing features" or, preferably, demonstrate the model's capability by actually introducing some missing values into the training data.
For example, you could add this after the train_test_split call:
# Introduce some missing values to demonstrate robustness
rng = np.random.default_rng(0)
missing_mask = rng.random(X_reg_train.shape) < 0.1
X_reg_train[missing_mask] = np.nan| X, y = make_regression( | ||
| n_samples=30, | ||
| n_features=4, | ||
| n_targets=2, | ||
| n_informative=4, | ||
| noise=0.2, | ||
| random_state=1, | ||
| ) |
There was a problem hiding this comment.
The PR description and example file mention handling of missing values, but the test data created here is complete. To properly verify that the multi-output wrappers work correctly with missing data, it would be beneficial to introduce some missing values (np.nan) into the X array. This would ensure that the underlying TabPFN estimator's missing value handling is correctly leveraged through the wrapper.
For example, you can add this line after the make_regression call:
X[::5, 0] = np.nan # Introduce some missing values
bejaeger
left a comment
bejaeger
left a comment
There was a problem hiding this comment.
Nice! Just have a few nits. Otherwise LGTM!
| random_state=1, | ||
| ) | ||
|
|
||
| X_clf = X_clf.astype(np.float32) |
There was a problem hiding this comment.
why convert here but not for regression above?
| @@ -0,0 +1,75 @@ | |||
| """Utilities for multi-output learning with TabPFN.""" | |||
There was a problem hiding this comment.
| """Utilities for multi-output learning with TabPFN.""" | |
| """Wrapper for multi-output learning with TabPFN.""" |
| ) -> dict[str, Any]: # pragma: no cover - delegating to sklearn | ||
| """Return parameters for this estimator with TabPFN kwargs included.""" | ||
| params = super().get_params(deep=deep) | ||
| if getattr(self, "_estimator_is_default", False): |
There was a problem hiding this comment.
| if getattr(self, "_estimator_is_default", False): | |
| if self._estimator_is_default: |
We always set the variable so would be a bit more readable.
| self, | ||
| estimator: _EstimatorT | None = None, | ||
| *, | ||
| n_jobs: int | None = None, |
There was a problem hiding this comment.
| n_jobs: int | None = None, | |
| n_preprocessing_jobs: int | None = None, |
We are deprecating n_jobs right now. Please change to n_preprocessing_jobs across the board.
2 participants
Summary
Testing
Codex Task