Skip to content
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

[Breaking Change] Remove last_dim_is_batch #2544

Merged
merged 6 commits into from
Aug 13, 2024
Merged

Conversation

gpleiss
Copy link
Member

@gpleiss gpleiss commented Jul 2, 2024

This PR (finally) bumps our PyTorch requirement to 2.0.

Most use cases are not affected. There are two breaking changes as part of this PR:

  1. Remove AdditiveStructureKernel and ProductStructureKernel. The functionality for these kernels can be implemented manually, and there is a new example notebook showing how to do so.
  2. Remove NewtonGirardAdditiveKernel. Again, the functionality of this kernel can be implemented manually, using the new sum_interaction_terms utility (which contains the the meat of the NewtonGirardAdditiveKernel anyways).

@gpleiss gpleiss requested a review from jacobrgardner July 2, 2024 23:31
@gpleiss gpleiss force-pushed the remove_last_dim_is_batch branch 2 times, most recently from 43f680d to 41a08c5 Compare July 2, 2024 23:42
@gpleiss
Copy link
Member Author

gpleiss commented Jul 2, 2024

Still todo: update example notebooks, and add new documentation on additive / product kernels.

@gpleiss gpleiss force-pushed the remove_last_dim_is_batch branch from 41a08c5 to e97d874 Compare July 2, 2024 23:55
@esantorella
Copy link
Collaborator

How are you thinking about warning users about breaking changes -- e.g. will there be deprecation warnings? (I do see that this is for merge into the develop branch.) Losing AdditiveStructureKernel and last_dim_is_batch will cause a few downstream breakages.

@gpleiss
Copy link
Member Author

gpleiss commented Jul 3, 2024

@esantorella good question, and this is a good question for other GPyTorch 2.0 breaking changes more broadly.

We could release a new minor release while we're still preparing 2.0 that adds deprecation warnings to notify users about breaking changes.

@jacobrgardner @JonathanWenger thoughts?

@JonathanWenger
Copy link
Collaborator

I think a minor release that has deprecations makes a lot of sense. It's not a lot of extra work to add the deprecations, and it might prevent some issues coming our way right after the planned 2.0 release.

@gpleiss gpleiss force-pushed the remove_last_dim_is_batch branch from e97d874 to 41f6ae8 Compare July 12, 2024 17:31
@gpleiss gpleiss changed the title [Breaking Change] WIP: Remove last_dim_is_batch [Breaking Change] Remove last_dim_is_batch Jul 12, 2024
@gpleiss gpleiss force-pushed the remove_last_dim_is_batch branch 2 times, most recently from bbea4f9 to 8d859b9 Compare July 17, 2024 17:09
@gpleiss gpleiss requested a review from JonathanWenger August 7, 2024 21:47
Copy link
Collaborator

@JonathanWenger JonathanWenger left a comment

Choose a reason for hiding this comment

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

Looks all good! Some minor documentation could still be polished (see comments).

gpytorch/kernels/grid_kernel.py Outdated Show resolved Hide resolved
gpytorch/kernels/linear_kernel.py Outdated Show resolved Hide resolved
**Note**: This is not a breaking change; "legacy" grids were deprecated
pre v1.0.
…Kernel

- The functionality of both kernels has not disappeared, but both
  kernels cannot work without the last_dim_is_batch_option.
- The examples/00_Basic_Usage/kernels_with_additive_or_product_structure.ipynb
  notebook describes how to replicate the functionality of both kernels
  without last_dim_is_batch.
- The functionality of this kernels has not disappeared, but this
  kernel cannot work without the last_dim_is_batch_option.
- The examples/00_Basic_Usage/kernels_with_additive_or_product_structure.ipynb
  notebook describes how to replicate the functionality of this kernel
  using the gpytorch.utils.sum_interaction_terms utility.
Addresses PR review comments
@gpleiss gpleiss force-pushed the remove_last_dim_is_batch branch from 8d859b9 to e4ec21b Compare August 13, 2024 17:25
@gpleiss gpleiss merged commit c18d95d into develop Aug 13, 2024
7 checks passed
@gpleiss gpleiss deleted the remove_last_dim_is_batch branch August 13, 2024 17:32
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.

3 participants