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

Fix kernel translation #311

Merged
merged 21 commits into from
Jul 17, 2024
Merged

Fix kernel translation #311

merged 21 commits into from
Jul 17, 2024

Conversation

AdrianSosic
Copy link
Collaborator

@AdrianSosic AdrianSosic commented Jul 12, 2024

This PR fixes one of the two issues that caused transfer learning performance to deteriorate, fixing several other issues along the way.

Bug fixes

  • Main issue fixed: the arguments provided to to_gpytorch (e.g. ard_num_dims) were not passed on to the GPyTorch kernel class.
  • The to_gpytorch call was not tested and did in fact not succeed in all cases. Causing issues:
    • Hypothesis strategies did not account for positive-valued attributes, and no validators where in place. --> fixed
    • The torch dtype was not correctly set when translating priors to GPyTorch. --> fixed using temporary workaround (see below)

Added workarounds

  • A proper solution to setting the default torch dtype requires overriding the torch import mechanism using importlibs machinery, due to lazy loading. A preliminary workaround is added in form of the set_default_torch_dtype utility, which manages the temporary code in a single place.

Improvements

  • The new match_attributes utility, which replaces filter_attributes now raises an error when attributes cannot be matched to the target callable, instead of silently dropping them. In strict=False mode, the unmatched arguments can be inspected as second return value for further processing.
  • Kernel translation to GPyTorch now ensures that all attributes are actually mapped
  • Added kernel validation, ensuring that invalid arguments trigger the expected errors
  • Added kernel translation tests, ensuring that all kernel components are correctly mapped to corresponding GPyTorch components

@AdrianSosic AdrianSosic added bug Something isn't working enhancement Expand / change existing functionality labels Jul 12, 2024
@AdrianSosic AdrianSosic self-assigned this Jul 12, 2024
Copy link
Collaborator Author

@AdrianSosic AdrianSosic left a comment

Choose a reason for hiding this comment

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

Hi @AVHopp, @Scienfitz, here a few open issues we might want to discuss.

baybe/utils/basic.py Outdated Show resolved Hide resolved
tests/test_kernels.py Show resolved Hide resolved
tests/test_kernels.py Outdated Show resolved Hide resolved
baybe/utils/basic.py Outdated Show resolved Hide resolved
tests/hypothesis_strategies/kernels.py Outdated Show resolved Hide resolved
tests/test_kernels.py Outdated Show resolved Hide resolved
tests/test_kernels.py Show resolved Hide resolved
Copy link
Collaborator

@AVHopp AVHopp left a comment

Choose a reason for hiding this comment

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

Minor comments. Will properly review in the following week. In any csae, kudos for spotting and fixing.

baybe/utils/basic.py Outdated Show resolved Hide resolved
baybe/utils/basic.py Show resolved Hide resolved
Copy link
Collaborator

@AVHopp AVHopp left a comment

Choose a reason for hiding this comment

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

Looks very nice, have some questions about why some things are necessary/whether I understand them correctly.

baybe/utils/basic.py Outdated Show resolved Hide resolved
baybe/kernels/base.py Outdated Show resolved Hide resolved
baybe/kernels/base.py Show resolved Hide resolved
tests/hypothesis_strategies/kernels.py Outdated Show resolved Hide resolved
tests/hypothesis_strategies/kernels.py Show resolved Hide resolved
tests/test_kernels.py Outdated Show resolved Hide resolved
tests/test_kernels.py Outdated Show resolved Hide resolved
tests/test_kernels.py Show resolved Hide resolved
tests/test_kernels.py Show resolved Hide resolved
tests/test_kernels.py Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
baybe/acquisition/base.py Outdated Show resolved Hide resolved
@AdrianSosic AdrianSosic merged commit 2edc551 into main Jul 17, 2024
10 checks passed
@AdrianSosic AdrianSosic deleted the bug/kernels branch July 17, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement Expand / change existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants