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

Improve efficiency of kernel calculation #55

Closed
wants to merge 3 commits into from
Closed

Improve efficiency of kernel calculation #55

wants to merge 3 commits into from

Conversation

joannajzou
Copy link
Member

@joannajzou joannajzou commented Dec 6, 2023

Small change to reduce the cost of computing symmetric kernel matrices. Only one half of the matrix terms [Kij] are computed, then copied over to [Kji].

The code is verified to return the same matrix as the original code.

Small change which reduces the cost of computing symmetric kernels. One half of the matrix terms [Kij] are computed, then copied over to [Kji].
Copy link
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

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

In theory we wouldn't even need to store the Lower triangle since it is being wrapped in Symmetric. Symmetric disregards the lower triangle already.

https://docs.julialang.org/en/v1/stdlib/LinearAlgebra/#LinearAlgebra.Symmetric

Note that Supper will not be equal to Slower unless A is itself symmetric (e.g. if A == transpose(A)).

@joannajzou joannajzou closed this Dec 6, 2023
@joannajzou joannajzou deleted the kernels branch December 6, 2023 16:17
@emmanuellujan
Copy link
Member

emmanuellujan commented Dec 6, 2023

@joannajzou To save memory allocations, I would try using SparseArrays. The current implementation still allocates the lower triangular part of the matrix.

@vchuravy
Copy link
Member

vchuravy commented Dec 6, 2023

For symmetric arrays the 2x memory overhead is better than the memory cost of a 50% filled sparse arrays. Remember that sparse arrays need to allocate two more int64 arrays of the same size for the indices.

@joannajzou I think your pull-request is still an improvement? Since it reduces the runtime of the initialization by half?

@emmanuellujan
Copy link
Member

For symmetric arrays the 2x memory overhead is better than the memory cost of a 50% filled sparse arrays. Remember that sparse arrays need to allocate two more int64 arrays of the same size for the indices.

True, but there should be an abstraction for sparse symmetric matrices somewhere, right? In problems related to clustering, where we need to compute a matrix of distances where N is of the order of millions, we need to save as much memory as possible. Do you know which is the proper abstraction to tackle this?

@joannajzou I think your pull-request is still an improvement? Since it reduces the runtime of the initialization by half?

It is

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.

3 participants