Enable upper for torch.linalg.cholesky#62434
Closed
walterddr wants to merge 3 commits intopytorch:masterfrom
Closed
Enable upper for torch.linalg.cholesky#62434walterddr wants to merge 3 commits intopytorch:masterfrom
walterddr wants to merge 3 commits intopytorch:masterfrom
Conversation
Contributor
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 49f485a (more details on the Dr. CI page):
❄️ 1 failure tentatively classified as flakybut reruns have not yet been triggered to confirm:
|
lezcano
reviewed
Jul 30, 2021
Collaborator
lezcano
left a comment
There was a problem hiding this comment.
This looks good, thanks!
Could you also add some tests to make sure the argument works as we expect?
Note that we have the identity cholesky(A, upper=True) == cholesky(A).transpose(-2, -1).conj() and an equivalent one for cholesky_ex.
Collaborator
There was a problem hiding this comment.
Perhaps
whether to return an upper triangular matrix. The tensor returned with
upper=Trueis the conjugate transpose of the tensor returned withupper=False.
Same in cholesky_ex.
6f704f6 to
96f41fb
Compare
walterddr
commented
Jul 30, 2021
Comment on lines
10191
to
10189
Contributor
Author
There was a problem hiding this comment.
these seems to be introducing backward incompatible changes to libtorch API (if i understand the CI job correctly). Should we keep those original signatures and redispatch them with default upper=False argument? did torch.cholesky have similar signature?
Collaborator
There was a problem hiding this comment.
torch.cholesky did have a similar signature. Any change in function signature in native_functions.yaml is marked as backward-incompatible change. If we want to ignore it for now, then an entry to allow_list in test/backward_compatibility/check_backward_compatibility.py should be added.
IvanYashchuk
approved these changes
Aug 3, 2021
96f41fb to
8852eaf
Compare
Contributor
|
@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Closed
facebook-github-bot
pushed a commit
that referenced
this pull request
Aug 4, 2021
Summary: a bug was discovered in #62434, for some reason comparing the schema name didn't match the allow_list item. So: 1. remove duplicate regex compile 2. make use of the schema string is used instead of just the name Pull Request resolved: #62687 Reviewed By: ezyang Differential Revision: D30102437 Pulled By: walterddr fbshipit-source-id: 541b2ed77948f24daebb08623cadabb034a241e0
e2729f2 to
cad0eb4
Compare
cad0eb4 to
49f485a
Compare
Contributor
|
@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Contributor
|
@walterddr merged this pull request in 3782f3e. |
4 tasks
mruberry
reviewed
Aug 11, 2021
|
|
||
| # "_ex" stands for experimental | ||
| - func: linalg_cholesky_ex(Tensor self, *, bool check_errors=False) -> (Tensor L, Tensor info) | ||
| - func: linalg_cholesky_ex(Tensor self, *, bool upper=False, bool check_errors=False) -> (Tensor L, Tensor info) |
Collaborator
There was a problem hiding this comment.
This is actually an FC-breaking change but it's probably OK because this function is so new
mruberry
reviewed
Aug 11, 2021
| upper (bool, optional): whether to return an upper triangular matrix. | ||
| The tensor returned with upper=True is the conjugate transpose of the tensor | ||
| returned with upper=False. | ||
| check_errors (bool, optional): controls whether to check the content of ``infos``. Default: `False`. |
Collaborator
|
cc @t-vi |
laurentdupin
pushed a commit
to laurentdupin/pytorch
that referenced
this pull request
Apr 25, 2026
Summary: a bug was discovered in pytorch#62434, for some reason comparing the schema name didn't match the allow_list item. So: 1. remove duplicate regex compile 2. make use of the schema string is used instead of just the name Pull Request resolved: pytorch#62687 Reviewed By: ezyang Differential Revision: D30102437 Pulled By: walterddr fbshipit-source-id: 541b2ed77948f24daebb08623cadabb034a241e0
laurentdupin
pushed a commit
to laurentdupin/pytorch
that referenced
this pull request
Apr 25, 2026
Summary: Fixes pytorch#61988 Pull Request resolved: pytorch#62434 Reviewed By: seemethere, tktrungna Differential Revision: D30079806 Pulled By: walterddr fbshipit-source-id: 044efb96525155c9bc7953ac4ad47c1b7c12fb20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #61988