KEMBAR78
use Vᴴ instead of Vh where appropriate in SVD docs by dagitses · Pull Request #61037 · pytorch/pytorch · GitHub
Skip to content

use Vᴴ instead of Vh where appropriate in SVD docs #61037

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
wants to merge 5 commits into from
Closed

use Vᴴ instead of Vh where appropriate in SVD docs #61037

wants to merge 5 commits into from

Conversation

@dagitses
Copy link
Collaborator

@dagitses dagitses commented Jun 30, 2021 •
edited
Loading

@dagitses dagitses mentioned this pull request Jun 30, 2021
Closed
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 30, 2021 •
edited
Loading

💊 CI failures summary and remediations

As of commit e37e522 (more details on the Dr. CI page and at hud.pytorch.org/pr/61037):

  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-scanned failure(s)

ci.pytorch.org: 1 failed

Preview docs built from this PR

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@dagitses dagitses closed this Jun 30, 2021
@dagitses dagitses deleted the gh/dagitses/1/head branch June 30, 2021 13:13
@dagitses dagitses restored the gh/dagitses/1/head branch June 30, 2021 13:27
@dagitses dagitses reopened this Jun 30, 2021
@dagitses
Copy link
Collaborator Author

@dagitses has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

1 similar comment
@dagitses
Copy link
Collaborator Author

@dagitses has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@dagitses
Copy link
Collaborator Author

@dagitses has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@dagitses
Copy link
Collaborator Author

@dagitses has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@dagitses dagitses requested a review from albanD June 30, 2021 17:32
albanD
albanD reviewed Jun 30, 2021
View reviewed changes
torch/linalg/__init__.py Outdated
The gradient will also be numerically unstable when :attr:`A` has small singular
values, as it also depends on the computaiton of :math:`\frac{1}{\sigma_i}`.
.. warning:: Backpropagation is only supported for input matrices with at most one zero eigenvalue.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like the warning above actually mentions most of this already. Sorry for missing that.
We should fix the formatting of Vh though to be V or V^H with math formatting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it looks like a similar warning exists in both variants of the function. So revert changes to both files and edit this file to use V^H instead of Vh? Should I do that across the board for this function or just the nearest warning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see some place already using Vᴴ. How does this look?

@dagitses dagitses changed the title warn about SVD outputs not supporting backprop use Vᴴ instead of Vh where appropriate in SVD docs Jul 8, 2021
albanD
albanD approved these changes Jul 9, 2021
View reviewed changes
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

LGTM!

@dagitses
Copy link
Collaborator Author

@dagitses has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@dagitses
Copy link
Collaborator Author

@dagitses has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@dagitses dagitses mentioned this pull request Jul 12, 2021
Closed
@dagitses
Copy link
Collaborator Author

@dagitses has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@dagitses
Copy link
Collaborator Author

@dagitses has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@dagitses merged this pull request in 5897a60.

@facebook-github-bot facebook-github-bot deleted the gh/dagitses/1/head branch July 16, 2021 14:17
@IvanYashchuk
Copy link
Collaborator

Note that when discussed improving linear algebra module's documentation it was proposed to use Unicode characters for mathematics, but we decided that LaTeX should be used instead. See #54878 (comment) and discussion in that issue.

Here in torch.linalg.svd Vh was used on purpose, it's the name of a returned variable, even though Vᴴ is a valid name for a Python variable, it could as well be named differently. NumPy uses vh, SciPy uses Vh. It can be confusing what the function actually returns when mixing mathematical expressions with the return signature.

cc: @lezcano

@IvanYashchuk IvanYashchuk 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 Jul 22, 2021
@lezcano
Copy link
Collaborator

lezcano commented Jul 22, 2021 •
edited
Loading

As @IvanYashchuk mentioned, there's both the discussion about not using unicode symbols in the docs due to problems in the rendering.

Now, the changes in this PR are not correct, as Vh refers to the name of a returned variable:

print(torch.linalg.svd(torch.rand(3, 4)).Vh)

This PR should be reverted cc @albanD @dagitses

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albanD albanD albanD approved these changes

Assignees

No one assigned

Labels

cla signed Merged 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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dagitses @facebook-github-bot @IvanYashchuk @lezcano @albanD