-
Notifications
You must be signed in to change notification settings - Fork 22
[WIP][New Model] TabFlex #171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
# FIXME: the below dependencies are not needed and not a required dependency for TabFlex | ||
# but is not lazy imported in the TabFlex code base. Thus, we monkey patch it to avoid | ||
# installing it. | ||
sys.modules["mlflow"] = types.ModuleType("mlflow") | ||
fake_gyptorch = types.ModuleType("gpytorch") | ||
fake_gyptorch.models = types.ModuleType("gpytorch.models") | ||
fake_gyptorch.models.ExactGP = ABC | ||
sys.modules["gpytorch"] = fake_gyptorch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this instead be done lazily in the AbstractModel implementation during fit prior to TabFlex import? Would this being here cause issues in other models potentially in future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be done before the import, so this could also be done lazily. But honestly, we should just fix the TICL code instead of doing this on our end.
@Innixma, thoughts about adding TabFlex to the LB even as authors have not at all responded so far? |
@LennartPurucker in general I think it is fine to merge (although I have some concerns with the import patches at start of file). Regarding adding to LB maybe we should add a "Author verified" column to indicate in which cases we synced with the authors and in which cases we did not / weren't able to reach them? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM assuming merge conflicts are resolved
@LennartPurucker can we move the import logic into the model class code so it doesn't impact other things? |
I will have to test it / tinker with it a bit after resolving the merge conflicts. |
Sounds like the best way forward for now. We can add this for TabDPT and BETA-PFN as well with various degrees. |
This code adds TabFlex (https://arxiv.org/abs/2506.05584) from https://github.com/microsoft/ticl
Benchmark TabArena (only Classification)
All results: tabflex.zip
Raw results: https://data.lennart-purucker.com/tabarena/data_TabFlex.zip
Notes
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.