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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

pstrf on positive semi-definite matrices #12435

Open
fKunstner opened this issue Oct 7, 2018 · 9 comments
Open

pstrf on positive semi-definite matrices #12435

fKunstner opened this issue Oct 7, 2018 · 9 comments
Labels
function request A request for a new function or the addition of new arguments/modes to an existing function. module: linear algebra Issues related to specialized linear algebra operations in PyTorch; includes matrix multiply matmul triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@fKunstner
Copy link

fKunstner commented Oct 7, 2018

馃悰 Bug

pstrf does not work on Positive Semi-Definite matrices while the documentation says it should.

The issue might be a misunderstanding on my part of the documentation.
Any correction to this misunderstanding is welcome :)

I am assuming that for M to be a Positive SemiDefinite matrix, it needs to hold that for any real vector x, x.t() @ M @ x >= 0,
whereas to be Positive Definite, it needs to hold that x.t() @ M @ x > 0.

To Reproduce

Using Pytorch 0.4.1, on CPU

import torch 
Zero = torch.tensor([0.0]).view(1,1)
print(torch.pstrf(Zero))

throws

RuntimeError: 
Lapack Error pstrf : 
matrix is rank deficient or not positive semidefinite at 
c:\programdata\miniconda3\conda-bld\pytorch_1532505617613\work\aten\src\th\generic/THTensorLapack.cpp:744

Expected behavior

pstrf is supposed to

Computes the pivoted Cholesky decomposition of a positive semidefinite matrix a. returns matrices u and piv.

In the example provided above, I would expected u should be [[0]] and piv should be [0]

Additional Details

I don't understand the error message,

matrix is rank deficient or not positive semidefinite

From my understanding, Positive SemiDefinite matrices that are not Positive Definite are by definition rank deficient.

cc @jianyuh @nikitaved @pearu @mruberry @heitorschueroff @walterddr @IvanYashchuk

@vishwakftw
Copy link
Contributor

I think pstrf requires a matrix with rank at least 1. I don鈥檛 think this is mentioned in the docs, will send in a patch.

@fKunstner
Copy link
Author

Interesting. However, it does not seem to work with a Rank-1, 2x2 matrix, unless I misunderstood your point.

import torch 
RankOne = torch.tensor([[1.0,0.0],[0.0,0.0]])
print(RankOne)
print(torch.pstrf(RankOne))
tensor([[1., 0.],
        [0., 0.]])
[...]
RuntimeError: 
Lapack Error pstrf : 
matrix is rank deficient or not positive semidefinite at
c:\programdata\miniconda3\conda-bld\pytorch_1532505617613\work\aten\src\th\generic/THTensorLapack.cpp:744

@vishwakftw
Copy link
Contributor

Oh, my bad. I thought that was the case. I will look into it in more detail.

@vishwakftw
Copy link
Contributor

vishwakftw commented Oct 8, 2018

I looked into it further, and there's nothing interfering with the LAPACK calls for pstrf. This probably means that there's some inaccuracy in the LAPACK implementation creeping in and playing spoilsport.

I also found out that pstrf doesn't have a CUDA implementation, which is an asymmetry.

I think we should just remove it. cc: @apaszke @soumith

@zou3519 zou3519 added the todo Not as important as medium or high priority tasks, but we will work on these. label Oct 8, 2018
@zou3519
Copy link
Contributor

zou3519 commented Oct 8, 2018

The matrix is rank deficient, which is correct, but the error message is indeed confusing. Maybe we should just make it say that the matrix is not positive definite (this is what scipy.linalg.cholesky says)

@vishwakftw
Copy link
Contributor

vishwakftw commented Oct 8, 2018

pstrf is meant for positive semi-definite matrices, meaning that there is something wrong going on with the errors. For positive definite we have potrf, which works fine I suppose.

@vishwakftw
Copy link
Contributor

Further more, do we need to have a function with only an implementation for the CPU. I think that would be inconvenient.

@ngimel
Copy link
Collaborator

ngimel commented Apr 13, 2021

Pytorch no longer has pstrf function. Labeling this for linalg in case we need to add support.

@ngimel ngimel added module: linear algebra Issues related to specialized linear algebra operations in PyTorch; includes matrix multiply matmul triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module and removed todo Not as important as medium or high priority tasks, but we will work on these. labels Apr 13, 2021
@mruberry mruberry added the function request A request for a new function or the addition of new arguments/modes to an existing function. label Apr 14, 2021
@lezcano
Copy link
Collaborator

lezcano commented Mar 25, 2022

Same as with the other issue, I think LDL provides a good enough solution for this. cc @IvanYashchuk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
function request A request for a new function or the addition of new arguments/modes to an existing function. module: linear algebra Issues related to specialized linear algebra operations in PyTorch; includes matrix multiply matmul triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

No branches or pull requests

6 participants