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 cosine dist #138

Closed
wants to merge 3 commits into from
Closed

Conversation

jandremarais
Copy link
Contributor

First attempt to add cosine distance metric as mentioned in #136

@edgarriba
Copy link
Member

@jandremarais thanks for the implementation. I'm considering wether this should be implemented more in terms of Tensor instead of Image. Which brings the question about how we set the axis direction where to compute the metric. What do you think ?

@jandremarais
Copy link
Contributor Author

I see. That makes more sense. Then it probably needs a "dim" parameter in the function too. Should it support broadcasting?

I found these implementation notes in the pytorch source interesting and leaving it here for reference:

https://github.com/pytorch/pytorch/blob/main/aten/src/ATen/native/Distance.cpp#L274-L303

@edgarriba
Copy link
Member

edgarriba commented Sep 20, 2024

i see, maybe we need even first a proper norm operator. Which I've been recently thinking about having a separated kornia-core-ops crate to disentagle the tensor memory and layout management to the rest of all the operators (even the existing ones). That was actually one of multiple reasons why decided to drop ndarray support to have a clean separated Tensor struct for the mere purpose of moving data round.

@jandremarais
Copy link
Contributor Author

Closing reasons:

  • Cosine sim should operate on tensors not images.
  • Cosine sim needs vector norm operation
  • Unsure where tensor operations should live

@edgarriba
Copy link
Member

Unsure where tensor operations should live

feel free to open a PR creating a new subcrate kornia-core-ops in the workspace

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.

2 participants