Skip to content

Conversation

@annaivagnes
Copy link
Contributor

Only replaced the torch.svd_lowrank method with torch.svd: the reason is that torch.svd_lowrank is randomized and may give different results in different runs (as specified in the documentation: https://pytorch.org/docs/stable/generated/torch.svd_lowrank.html)

@dario-coscia
Copy link
Collaborator

Thanks @annaivagnes ! Maybe we can add in the init a boolean kwarg called 'randomizedwhich if true usestorch.svd_lowrankotherwisetorch.svd. Like this we can use both, I think torch.svd_lowrank` it is a bit faster in CUDA

:param torch.Tensor X: The tensor to be reduced.
"""
if X.device.type == "mps": # svd_lowrank not arailable for mps
Copy link
Collaborator

Choose a reason for hiding this comment

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

So here would be something like:

        if X.device.type == "mps":  #  svd_lowrank not arailable for mps
            self._basis = torch.svd(X.T)[0].T
        else:
            self._basis = self.svd(X.T, q=X.shape[0])[0].T

We should put a warning on the doc for MPS users maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reviewing!

In my opinion, if we want to keep the svd_lowrank to speed up the computations, then we should set up a seed or give a Warning saying that the method is randomized and that the computation of the discretized basis may differ in different runs. Also what you suggested with the randomized kwarg is ok (I would include the Warning anyway, maybe).

Then, maybe for MPS users the Warning can be The POD is computed using the standard SVD approach and this may slow down the computation

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, then let's do it like this. We can put a warning at initialization, and if an mps user is using the layer we raise another waning like the one you described :)

Copy link
Collaborator

@dario-coscia dario-coscia left a comment

Choose a reason for hiding this comment

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

Thanks Anna!

@dario-coscia dario-coscia merged commit c28a4bd into mathLab:master Feb 14, 2025
15 of 16 checks passed
AleDinve pushed a commit that referenced this pull request Mar 17, 2025
* Change SVD type in pod.py
dario-coscia pushed a commit that referenced this pull request Apr 17, 2025
* Change SVD type in pod.py
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