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

fixed rbf kernel in cka #175

Merged
merged 4 commits into from
May 19, 2024
Merged

fixed rbf kernel in cka #175

merged 4 commits into from
May 19, 2024

Conversation

LukasMut
Copy link
Collaborator

@LukasMut LukasMut commented May 17, 2024

fixed sigma arg in rbf kernel

@LukasMut LukasMut added bug Something isn't working cleanup Deprecate or refactor code labels May 17, 2024
@LukasMut LukasMut requested review from a1247418 and jonasd4 May 17, 2024 12:38
@LukasMut LukasMut self-assigned this May 17, 2024
Copy link

codecov bot commented May 17, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 76.42%. Comparing base (16940b8) to head (40099bb).

Current head 40099bb differs from pull request most recent head 0a8789d

Please upload reports for the commit 0a8789d to get more accurate results.

Files Patch % Lines
thingsvision/core/cka/cka_numpy.py 66.66% 0 Missing and 1 partial ⚠️
thingsvision/core/cka/cka_torch.py 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
+ Coverage   76.40%   76.42%   +0.02%     
==========================================
  Files          40       40              
  Lines        2051     2053       +2     
  Branches      261      261              
==========================================
+ Hits         1567     1569       +2     
  Misses        400      400              
  Partials       84       84              
Flag Coverage Δ
unittests 76.42% <60.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@a1247418 a1247418 left a comment

Choose a reason for hiding this comment

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

LGTM

@LukasMut LukasMut requested a review from lciernik May 19, 2024 06:53
Copy link
Collaborator

@lciernik lciernik left a comment

Choose a reason for hiding this comment

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

Left one question otherwise LGTM

"""Use an rbf kernel for computing the gram matrix. Sigma defines the width."""
GX = X @ X.T
KX = np.diag(GX) - GX + (np.diag(GX) - GX).T
if sigma is None:
if self.sigma is None:
mdist = np.median(KX[KX != 0])
sigma = math.sqrt(mdist)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question, if self.sigma=None, you compute it's value, but shall it be stored? Or shall we recompute it every time?

Copy link
Collaborator Author

@LukasMut LukasMut May 19, 2024

Choose a reason for hiding this comment

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

We could store it but the caveat here is that sigma depends on the input variable X which changes as soon as we use the feature space Y instead of X. I am not sure it's worth storing two sigmas, one for X and one for Y, if self.sigma=None. Also, self.sigma=None should be seen as a special case. Ideally, one assigns self.sigma a specific value which is then used for the bandwidth of the rbf kernel irrespective of the feature space.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, agree

@LukasMut LukasMut added this pull request to the merge queue May 19, 2024
Merged via the queue into master with commit ab01641 May 19, 2024
3 checks passed
@LukasMut LukasMut deleted the cka_fix branch May 19, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cleanup Deprecate or refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants