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

Reduce memory required for Fourier transform. #95

Merged
merged 2 commits into from
Mar 20, 2021

Conversation

pulquero
Copy link

Fixes #94 .

@@ -171,7 +171,7 @@ def compute_fourier_basis(self, n_eigenvectors=None):

# TODO: handle non-symmetric Laplacians. Test lap_type?
if n_eigenvectors == self.n_vertices:
self._e, self._U = np.linalg.eigh(self.L.toarray())
self._e, self._U = linalg.eigh(self.L.toarray(order='F'), overwrite_a=True)
Copy link
Author

Choose a reason for hiding this comment

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

order='F' as lapack is fortran ordered, and re-use space for eigenvectors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! Does the Fortran order make the EVD faster?

Copy link
Author

Choose a reason for hiding this comment

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

It should avoid the matrix being transposed from C order to F order when it is pushed out to lapack.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense, thanks. At some point we should do a proper benchmark and find the best settings for the partial and full EVDs.

@mdeff
Copy link
Collaborator

mdeff commented Mar 20, 2021

Rebased to fix CI.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.557% when pulling e4eabbe on pulquero:mem-reduction into a00e8a2 on epfl-lts2:master.

@mdeff mdeff merged commit a7c627e into epfl-lts2:master Mar 20, 2021
@mdeff
Copy link
Collaborator

mdeff commented Mar 20, 2021

Thanks for the improvement!

@mdeff mdeff mentioned this pull request Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Memory usage reduction
3 participants