Skip to content

Fix orgqr input size conditions #42825

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

Fix orgqr input size conditions #42825

kurtamohler wants to merge 2 commits into from

Conversation

@kurtamohler
Copy link
Collaborator

@kurtamohler kurtamohler commented Aug 10, 2020 •
edited
Loading

  • Adds support for n > k
  • Throw error if m >= n >= k is not true
  • Updates existing error messages to match argument names shown in public docs
  • Adds error tests

Fixes #41776

@kurtamohler kurtamohler requested review from albanD and mattip August 10, 2020 19:28
@kurtamohler kurtamohler force-pushed the orgqr-size-conditions-fix branch from e8cbeb8 to 83ca96b Compare August 10, 2020 19:38
@dr-ci
Copy link

dr-ci bot commented Aug 10, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit 4bf9557 (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 3 times.

albanD
albanD reviewed Aug 10, 2020
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.

Thanks for the PR!

There was no actual check that n==k before?

@mattip
Copy link
Contributor

mattip commented Aug 10, 2020

There was no actual check that n==k before?

Correct. The normative use for this function is as a chained function from the output of geqrf, i.e., Q = torch.orgqr(torch.geqrf(input)) so the current tests do not consider it separately from geqrf

mattip
mattip approved these changes Aug 10, 2020
View reviewed changes
Copy link
Contributor

@mattip mattip left a comment

Choose a reason for hiding this comment

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

LGTM. There is only CPU dispatch for this function, so we don't have to worry about the CUDA equivalent.

@smessmer smessmer added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 10, 2020
albanD
albanD approved these changes Aug 10, 2020
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.

Sounds good. Thanks.

facebook-github-bot
facebook-github-bot reviewed Aug 10, 2020
View reviewed changes
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.

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

facebook-github-bot
facebook-github-bot reviewed Aug 10, 2020
View reviewed changes
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.

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

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 11, 2020

@albanD merged this pull request in 2c8cbd7.

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

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@albanD albanD albanD approved these changes

+1 more reviewer

@mattip mattip mattip approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged open source 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

Successfully merging this pull request may close these issues.

torch.orgqr aborts and core dump

7 participants

@kurtamohler @mattip @facebook-github-bot @albanD @smessmer @pytorchbot @mruberry