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

Unnecessary overhead when calculating CCALoss #191

Open
JH2k00 opened this issue Feb 10, 2024 · 5 comments
Open

Unnecessary overhead when calculating CCALoss #191

JH2k00 opened this issue Feb 10, 2024 · 5 comments

Comments

@JH2k00
Copy link

JH2k00 commented Feb 10, 2024

Greetings,

I was looking at the CCALoss' current implementation and wondered why torch.cov was called 3 times, when one would have sufficed. The following Covariance Matrix already contains SigmaHat11 and SigmaHat22 :

SigmaHat12 = torch.cov(torch.hstack((representations[0], representations[1])).T)

I ran a simple test with artificial representations to confirm my thoughts :

import torch

o1 = 5
o2 = 3

N = 10
representations = [torch.randn(size=(N, o1)), torch.randn(size=(N, o2))]

representations = [
        representation - representation.mean(dim=0)
        for representation in representations
    ]

SigmaHat = torch.cov(
            torch.hstack((representations[0], representations[1])).T
        )

SigmaHat12 = SigmaHat[:o1, o1:]

SigmaHat11_approx = SigmaHat[:o1, :o1]
SigmaHat22_approx = SigmaHat[-o2:, -o2:]

SigmaHat11 = torch.cov(representations[0].T)

SigmaHat22 = torch.cov(representations[1].T)

print(torch.allclose(SigmaHat11, SigmaHat11_approx))
print(torch.allclose(SigmaHat22, SigmaHat22_approx))

And this test returns True for both print statements. Am I missing something or could one remove the two unnecessary torch.cov calls ?

The torch.nn.LeakyReLU() call in eigvals = torch.nn.LeakyReLU()(eigvals[torch.gt(eigvals, 0)]) also seems unnecessary to me, shouldn't all values in eigvals[torch.gt(eigvals, 0)] already be positive ?

Thank you for your time and help.

Best regards,
JH

@jameschapman19
Copy link
Owner

jameschapman19 commented Feb 10, 2024

You're correct on the cov calls

@jameschapman19
Copy link
Owner

jameschapman19 commented Feb 10, 2024

tl;dr you're right on the leaky relu theoretically speaking and probably practically too.

I agree that all the eigenvalues should be positive semidefinite (everything is a covariance matrix, covariance matrices are PSD etc.)

However sometimes they aren't numerically psd with the torch cov solver which sometimes makes things freak out, in particular when using minibatch size ~= latent dimensions (see the issues on literally all the cca implementations on GitHub).

The reason the code is how it is now is that I once hypothesised before I understood the math that the leaky relu might help and maybe it does maybe it doesn't but my view now is for the original CCA loss you need minibatch size >> latent dimensions for things to work well which means in practice the eigenvalues are positive so relu==leakyrelu

For all DCCA, I recommend our CCA-EY loss which is awesome give it a go and you'll get better results than the original CCA one 🫡

@JH2k00
Copy link
Author

JH2k00 commented Feb 10, 2024

Sounds good, thanks for your speedy answer and the tip with the CCA-EY loss, I'll definitely try it out :D
Are you planning on updating the original CCA loss, or should I close the issue ?

@jameschapman19
Copy link
Owner

Yeah will do when I get a moment

@JH2k00
Copy link
Author

JH2k00 commented Feb 10, 2024

Alright, thank you for the great repository :D

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

No branches or pull requests

2 participants