8000 Doc bug about torch.linalg.inv · Issue #77954 · pytorch/pytorch · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Doc bug about torch.linalg.inv #77954

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.

Sign up for GitHub

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

Jump to bottom
Closed
zhongshsh opened this issue May 20, 2022 · 10 comments
Closed

Doc bug about torch.linalg.inv #77954

zhongshsh opened this issue May 20, 2022 · 10 comments
Labels
module: docs Related to our documentation, both in docs/ and docblocks 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

@zhongshsh
Copy link
zhongshsh commented May 20, 2022 •
edited by pytorch-bot bot
Loading

📚 The doc issue

https://pytorch.org/docs/stable/generated/torch.linalg.inv.html

In Examples, line 7 ends with an extra bracket.

A = torch.randn(4, 4)
Ainv = torch.linalg.inv(A)
torch.dist(A @ Ainv, torch.eye(4))

A = torch.randn(2, 3, 4, 4)  # Batch of matrices
Ainv = torch.linalg.inv(A)
torch.dist(A @ Ainv, torch.eye(4)))

A = torch.randn(4, 4, dtype=torch.complex128)  # Complex matrix
Ainv = torch.linalg.inv(A)
torch.dist(A @ Ainv, torch.eye(4))

Suggest a potential alternative/fix

A = torch.randn(4, 4)
Ainv = torch.linalg.inv(A)
torch.dist(A @ Ainv, torch.eye(4))

A = torch.randn(2, 3, 4, 4)  # Batch of matrices
Ainv = torch.linalg.inv(A)
torch.dist(A @ Ainv, torch.eye(4))

A = torch.randn(4, 4, dtype=torch.complex128)  # Complex matrix
Ainv = torch.linalg.inv(A)
torch.dist(A @ Ainv, torch.eye(4))

cc @svekars @holly1238 @jianyuh @nikitaved @pearu @mruberry @walterddr @IvanYashchuk @xwang233 @lezcano
@lezcano
Copy link
Collaborator
lezcano commented May 20, 2022

Yup. We'd accept a fix for this.
Even more, the linalg docs (or all docs for the matter) could do with a sanitiser, starting checking that open and closing parents agree.

@lezcano lezcano added module: docs Related to our documentation, both in docs/ and docblocks module: linear algebra Issues related to specialized linear algebra operations in PyTorch; includes matrix multiply matmul labels May 20, 2022
@anjali411 anjali411 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 23, 2022
@zhongshsh
Copy link
Author
zhongshsh commented May 23, 2022 •
edited
Loading

In addition, there are some mistakes in the details.

In pytorch/aten/src/ATen/native/LinearAlgebraUtils.h,

// Validates input shapes for operations on batches of square matrices (inverse, cholesky, symeig, eig)
static inline void squareCheckInputs(const Tensor& self, const char* const f_name) {
  TORCH_CHECK(self.dim() >= 2, f_name, ": The input tensor must have at least 2 dimensions.");
  TORCH_CHECK(self.size(-1) == self.size(-2),
              f_name,
              ": A must be batches of square matrices, "
              "but they are ", self.size(-1), " by ", self.size(-2), " matrices");
}

And in pytorch/torch/_meta_registrations.py,

def squareCheckInputs(self, f_name):
    assert self.dim() >= 2, f"{f_name}: The input tensor must have at least 2 dimensions."
    # TODO: I think the error message has the -2 and -1 swapped.  If you fix
    # it fix the C++ squareCheckInputs too
    assert self.size(-1) == self.size(-2), \
        f"{f_name}: A must be batches of square matrices, but they are {self.size(-1)} by {self.size(-2)} matrices"

Maybe they should be {self.size(-2)} by {self.size(-1)}.

@zhongshsh
Copy link
Author

You'd better judge singular matrices, otherwise this operator will calculate incorrectly. Example:

Python 3.9.7 (default, Sep 16 2021, 13:09:58) 
[GCC 7.5.0] :: Anaconda, Inc. on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import torch
>>> A = torch.tensor([[4.1, 2.8], [9.676, 6.608]])
>>> Ainv = torch.linalg.inv(A)
>>> Ainv
tensor([[ 22473374.,  -9522616.],
        [-32907440.,  13943831.]])
>>> torch.dist(A @ Ainv, torch.eye(2))
tensor(4.2426)

@lezcano
Copy link
Collaborator
lezcano commented May 23, 2022

Re. squareCheckInputs, you are completely right. I'm impressed we never realised of that (@nikitaved @IvanYashchuk @mruberry)

Re. singular matrices, we do try to throw an error when the matrix is singular, but sometimes I guess that it fails and we don't catch it given that detecting that a matrix is exactly singular. For example, that exact matrix does fail on my machine throwing a:

torch._C._LinAlgError: torch.linalg.inv: The diagonal element 2 is zero, the inversion could not be completed because the input matrix is singular

while it succeeds on CUDA. As such, if you want to invert matrices that may or may not be invertible, you'd be better off doing some checks as computing the distance to torch.eye, or doing lu_factor + lu_solve and look at the diagonal of U see if it could potentially be singular.

I plan to improve both the efficiency and the stability of linalg.inv by adding another PR to the stack #77634 but I haven't had the time yet :(

@zhongshsh
Copy link
Author
zhongshsh commented May 23, 2022 •
edited
Loading

Thanks for your reply. If you want to fix linalg.inv, maybe this PR will give you some ideas.

@lezcano
Copy link
Collaborator
lezcano commented May 23, 2022

We will implement torch.linalg.inv(A) as torch.solve(A, torch.eye(A.size(-1), device=A.device())) pretty much.

@zhongshsh
Copy link
Author

I tested this method, and it didn't solve the original problem.

Python 3.9.7 (default, Sep 16 2021, 13:09:58) 
[GCC 7.5.0] :: Anaconda, Inc. on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import torch
>>> A = torch.tensor([[4.1, 2.8], [9.676, 6.608]])
>>> Ainv, LU = torch.solve(torch.eye(A.size(-1), device=A.device), A)
>>> Ainv
tensor([[ 22473374.,  -9522616.],
        [-32907440.,  13943831.]])
>>> torch.dist(A @ Ainv, torch.eye(A.size(-1)))
tensor(4.2426)

@lezcano
Copy link
Collaborator
lezcano commented May 23, 2022

We don't aim to solve the original problem, as we don't implement the backends for these functions. Note that detecting whether a matrix is singular is a very difficult problem because we don't work with exact precision. If you do care about these things in your application, you should make sure that the matrix that you pass the solver (linalg.solve or linalg.inv) is well-conditioned.

That being said, we could update the docs to be a bit less explicit about what happens when the matrix is almost singular.

@zhongshsh
Copy link
Author
zhongshsh commented May 24, 2022 •
edited
Loading

Alright, I get the message.

@lezcano
Copy link
Collaborator
lezcano commented Jun 20, 2022

Fixed in master in #77079

@lezcano lezcano closed this as completed Jun 20, 2022
@lezcano lezcano mentioned this issue Oct 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
module: docs Related to our documentation, both in docs/ and docblocks 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
Milestone
No milestone
Development

No branches or pull requests
3 participants
@lezcano @anjali411 @zhongshsh
0