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

Added computing matrix condition numbers (linalg.cond) #45832

Closed
wants to merge 53 commits into from

Conversation

IvanYashchuk
Copy link
Collaborator

@IvanYashchuk IvanYashchuk commented Oct 5, 2020

This PR adds torch.linalg.cond for NumPy compatibility.

Ref #42666.

@IvanYashchuk IvanYashchuk added module: numpy Related to numpy support, and also numpy compatibility of our operators module: linear algebra Issues related to specialized linear algebra operations in PyTorch; includes matrix multiply matmul labels Oct 5, 2020
Now for p=None and p=±2 condition number is computed with svd.
Added errorchecks.
@facebook-github-bot
Copy link
Contributor

Hi @IvanYashchuk!

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but we do not have a signature on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@IvanYashchuk IvanYashchuk marked this pull request as ready for review November 3, 2020 11:56
@dr-ci
Copy link

dr-ci bot commented Nov 3, 2020

💊 CI failures summary and remediations

As of commit 898f855 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 78 times.

@ejguan ejguan added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Nov 4, 2020
continue
run_test_case(a, ord)

# TODO: once "inverse_cuda" supports complex dtypes, they shall be added to above tests
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#45034 got merged. This needs to be updated.

squareCheckInputs(self);
std::array<int64_t, 2> dim_arr = {-2, -1};
optional<IntArrayRef> dim = IntArrayRef(dim_arr);
// Ignore errors if not invertible, result is INFINITY in this case
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably should add a note here that if in the future it is decided that inverse routine shouldn't throw an error by default (for performance reasons). Then the behaviour here could be changed that the inverse matrix contains NaNs and the norm of such matrix is also NaN, and apply at::nan_to_num(result, INFINITY); to get the result.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll keep throwing errors by default but add _ex variants that return the errors instead of throwing them.

@IvanYashchuk
Copy link
Collaborator Author

@mruberry, this PR is ready for starting the review process.
It adds torch.linalg.cond a function that computes the condition number, that is given a matrix A compute norm(A)*norm(inverse(A)).
out= variant is implemented. Autograd and JIT tests pass as well.

torch/linalg/__init__.py Outdated Show resolved Hide resolved
@IvanYashchuk
Copy link
Collaborator Author

@mruberry, I updated this pull request:

  • Modified the implementation to return zero for 0x0 matrices and added tests for that.
  • Added checks whether the provided norm type p is valid.
  • Updated documentation to include a reference to svd for explaining why non-square inputs are allowed for p={None, -2, 2}

torch/linalg/__init__.py Outdated Show resolved Hide resolved
torch/linalg/__init__.py Outdated Show resolved Hide resolved
@mruberry
Copy link
Collaborator

mruberry commented Dec 2, 2020

Update looks pretty good, @IvanYashchuk, just a last couple doc tweaks.

@IvanYashchuk
Copy link
Collaborator Author

@mruberry, I updated the docs per your suggested tweaks.

For other norms, however, :attr:`input` must be a square matrix or a batch of square matrices,
and if this requirement is not satisfied a RuntimeError will be thrown.

.. note:: If :attr:`input` is a non-invertible matrix then a tensor containing infinity will be returned.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Darn, I just realized this note will need to be updated, too, since non-invertbility doesn't apply to both cases, and the next sentence is redundant with the previous note.

Sorry I missed that on the last pass, @IvanYashchuk.

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 updated this note mentioning that it applies only for some p.

.. note:: For ``p = {'fro', 'nuc', inf, -inf, 1, -1}`` if :attr:`input` is a non-invertible matrix then
          a tensor containing infinity will be returned. If :attr:`input` is a batch of matrices and one
          or more of them is not invertible then a RuntimeError will be thrown.

Specified that this note applies only for p = {'fro', 'nuc', inf, -inf, 1, -1}
@mruberry
Copy link
Collaborator

mruberry commented Dec 3, 2020

Lint failure looks real, unfortunately:

{
    path: 'torch/linalg/__init__.py',
    start_line: 354,
    end_line: 354,
    start_column: 141,
    end_column: 141,
    annotation_level: 'failure',
    message: '[B950] line too long (140 > 120 characters)'
  }

@IvanYashchuk
Copy link
Collaborator Author

@mruberry I'm sorry about that. Seems like I need to install pre-commit hooks to avoid these situations.
Now it's fixed.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Contributor

@mruberry merged this pull request in cb28508.

facebook-github-bot pushed a commit that referenced this pull request Dec 8, 2020
Summary:
This PR makes documentation for `cond` available at https://pytorch.org/docs/master/linalg.html
I forgot to include this change in #45832.

Pull Request resolved: #48941

Reviewed By: ngimel

Differential Revision: D25379244

Pulled By: mruberry

fbshipit-source-id: c8c0a0b8a05c17025d6c3cea405b2add369e2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged module: linear algebra Issues related to specialized linear algebra operations in PyTorch; includes matrix multiply matmul module: numpy Related to numpy support, and also numpy compatibility of our operators open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants