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

Improve torch.linalg.qr #50046

Closed
wants to merge 13 commits into from
Closed

Conversation

antocuni
Copy link
Contributor

@antocuni antocuni commented Jan 4, 2021

This is a follow up of PR #47764 to fix the remaining details.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jan 4, 2021

💊 CI failures summary and remediations

As of commit 92b0225 (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 to the (internal) Dr. CI Users group.

This comment has been revised 13 times.

@mruberry mruberry self-requested a review January 4, 2021 16:10
@mruberry mruberry added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jan 4, 2021
@codecov
Copy link

codecov bot commented Jan 4, 2021

Codecov Report

Merging #50046 (92b0225) into master (e4d596c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #50046   +/-   ##
=======================================
  Coverage   80.66%   80.66%           
=======================================
  Files        1899     1899           
  Lines      206066   206066           
=======================================
+ Hits       166224   166225    +1     
+ Misses      39842    39841    -1     

@antocuni antocuni marked this pull request as ready for review January 4, 2021 20:50
@antocuni antocuni requested a review from albanD as a code owner January 4, 2021 20:50
@antocuni
Copy link
Contributor Author

antocuni commented Jan 4, 2021

@mruberry as promised, here is a follow up PR to fix all the remaining details of PR #47764.
I think I addressed all of your comments apart from this which I didn't understand:
#47764 (review)

(tests are still running, I'll check their status tomorrow)

@antocuni
Copy link
Contributor Author

antocuni commented Jan 5, 2021

@mruberry tests seem to be ok and the failure unrelated so it's ready for review

Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

Awesome!

This looks great. The remaining issue is pretty niche. Let's not worry about it for now. When we review the torch.linalg functions ahead of the 1.8 release we can see if there are additional tweaks, then.

@mruberry
Copy link
Collaborator

mruberry commented Jan 6, 2021

Docs failures actually are real, although they're very hard to parse:

Jan 05 15:57:22 docstring of torch.linalg.qr:29: WARNING: Explicit markup ends without a blank line; unexpected unindent.

Just ping me when this is ready to merge.

@antocuni
Copy link
Contributor Author

antocuni commented Jan 7, 2021

Just ping me when this is ready to merge.

fixed the issue, the docs jobs are now green. Other jobs are still in-progress though, I'll take care to ping you when they are green for real

@antocuni
Copy link
Contributor Author

antocuni commented Jan 7, 2021

@mruberry all tests are green 👍

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 b5ab0a7.

hwangdeyu pushed a commit to hwangdeyu/pytorch that referenced this pull request Jan 14, 2021
Summary:
This is a follow up of PR pytorch#47764 to fix the remaining details.

Pull Request resolved: pytorch#50046

Reviewed By: zou3519

Differential Revision: D25825557

Pulled By: mruberry

fbshipit-source-id: b8e335e02265e73484a99b0189e4cc042828e0a9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged 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

4 participants