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

Add support for diagonal Kronecker factors in Kron matrix class #136

Merged
merged 4 commits into from
Aug 16, 2023

Conversation

runame
Copy link
Collaborator

@runame runame commented Aug 11, 2023

To support diagonal Kronecker factors in this library we have to adopt the methods of the Kron matrix class to be able to handle this correctly.

@runame runame added the enhancement New feature or request label Aug 11, 2023
Copy link
Owner

@aleximmer aleximmer left a comment

Choose a reason for hiding this comment

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

LGTM, just some suggestions and questions. The clone() before was, if I remember correctly, important or necessary to maintain differentiability when it's needed. I am not sure whether torch.tensor(h) guarantees the same.

else:
# Diagonal Kronecker factor.
l = Hi
# This might be too memory intensive since len(Hi) can be large.
Copy link
Owner

Choose a reason for hiding this comment

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

Remove this comment that is from sorting previously?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No it is due to the torch.eye, since the Kronecker factor is diagonal it might be too large to explicitly build a len(H_i) x len(H_i) matrix.

laplace/utils/matrix.py Outdated Show resolved Hide resolved
laplace/utils/matrix.py Outdated Show resolved Hide resolved
laplace/utils/matrix.py Outdated Show resolved Hide resolved
tests/test_baselaplace.py Show resolved Hide resolved
tests/test_baselaplace.py Show resolved Hide resolved
@runame
Copy link
Collaborator Author

runame commented Aug 16, 2023

Thanks! The initialisation is now not using tensors but just float scalars, so clone() is not possible anymore. Not sure if this breaks something.

@aleximmer
Copy link
Owner

If it passes the tests, I guess we have to find out later.

@aleximmer aleximmer merged commit ac44863 into mc-subset2 Aug 16, 2023
@runame runame deleted the kron-init branch August 17, 2023 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants