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

Fix a typo in cholesky_inverse documentation #110364

Closed

Conversation

raphaelreme
Copy link
Contributor

@raphaelreme raphaelreme commented Oct 1, 2023

Very small PR to fix a typo in https://pytorch.org/docs/stable/generated/torch.cholesky_inverse.html doc.

According to the current doc, the function expects $A$, the symmetric positive-definite matrix, as input. But the examples given (and more important, the code) is using $u$ the cholesky decomposition of this matrix (like cholesky_solve).

Also, it provides a correct example of batch usage of this function.

cc @svekars @carljparker

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 1, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@pytorch-bot pytorch-bot bot added the release notes: python_frontend release notes category label Oct 1, 2023
@pytorch-bot
Copy link

pytorch-bot bot commented Oct 1, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/110364

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 30d220e with merge base 13af952 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

torch/_torch_docs.py Outdated Show resolved Hide resolved
input (Tensor): the input tensor :math:`A` of size :math:`(*, n, n)`,
consisting of symmetric positive-definite matrices
where :math:`*` is zero or more batch dimensions.
input (Tensor): input matrix :math:`u` of size :math:`(*, m, m)`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why changing n into m tensor into matrix?
Please use the nomenclature used in the rest of the linalg module, like in cholesky for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the same description as in cholesky_solve which shares exactly the same parameter u

But it is true it could be n instead of m, as cholesky uses n and not m but cholesky_solve should probably follow the same nomenclature.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The docs for these ops are not particularly clean, as we chose not to port them to linalg. The linalg docs are properly curated, so it's best to use them as the reference in style.

If, as a follow up, you want to submit a PR writing the docs for these two ops following the styles of torch.linalg, I'd be more than happy to review it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a commit that rewrites these two ops following what is done in linalg.cholesky. I let you have a look to the new modifications

Copy link
Collaborator

@lezcano lezcano left a comment

Choose a reason for hiding this comment

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

Absolutely banging!

Left a few minor comments, but otherwise LGTM. Let me trigger CI to make sure that everything renders correctly, and that the doc linter does not complain.

torch/_torch_docs.py Outdated Show resolved Hide resolved
torch/_torch_docs.py Outdated Show resolved Hide resolved
torch/_torch_docs.py Outdated Show resolved Hide resolved
torch/_torch_docs.py Outdated Show resolved Hide resolved
@lezcano lezcano added module: docs Related to our documentation, both in docs/ and docblocks topic: docs topic category labels Oct 3, 2023
@lezcano lezcano self-requested a review October 3, 2023 13:36
Copy link
Collaborator

@lezcano lezcano left a comment

Choose a reason for hiding this comment

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

A few minor points. Otherwise, the docs look great!

Once these are addressed it should be ready to be merged.

torch/_torch_docs.py Outdated Show resolved Hide resolved
torch/_torch_docs.py Outdated Show resolved Hide resolved
torch/_torch_docs.py Outdated Show resolved Hide resolved
torch/_torch_docs.py Outdated Show resolved Hide resolved
torch/_torch_docs.py Outdated Show resolved Hide resolved
@lezcano
Copy link
Collaborator

lezcano commented Oct 4, 2023

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 4, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged module: docs Related to our documentation, both in docs/ and docblocks open source release notes: python_frontend release notes category topic: docs topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants