Skip to content

ger is an alias to outer, not the other way around#59448

Closed
albanD wants to merge 2 commits intogh/albanD/94/basefrom
gh/albanD/94/head
Closed

ger is an alias to outer, not the other way around#59448
albanD wants to merge 2 commits intogh/albanD/94/basefrom
gh/albanD/94/head

Conversation

@albanD
Copy link
Copy Markdown
Collaborator

@albanD albanD commented Jun 4, 2021

Stack from ghstack:

Differential Revision: D28900487

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Jun 4, 2021

💊 CI failures summary and remediations

As of commit e1da5df (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.

Click here to manually regenerate this comment.

@albanD albanD requested review from mruberry and zou3519 June 4, 2021 15:17
self: grad.mv(vec2.conj())
vec2: grad.t().mv(self.conj())
- name: outer(Tensor self, Tensor vec2) -> Tensor
self: grad.mm(vec2.unsqueeze(-1).conj()).squeeze(-1)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I had to modify this formula because outer supports half dtype right now but mv does not.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should probably add a comment and file an issue about replacing the formula with mv if/when it ever gets half support...

Are there any performance concerns here? I'm not too worried about 2 extra views here and I've rarely seen ger/outer used in the wild

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Actually due to more issues with cuda devices, I'll completely remove that formula and let this be autodiffed.
Note that outer was already autodiffed so that shouldn't change anything for this one.
And ger is deprecated so people should not be using it.

@albanD
Copy link
Copy Markdown
Collaborator Author

albanD commented Jun 4, 2021

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

@mruberry
Copy link
Copy Markdown
Collaborator

mruberry commented Jun 6, 2021

Nice fix, @albanD! Looks like @zou3519 is helping with the review, but let me know if you have a question about aliasing

@albanD
Copy link
Copy Markdown
Collaborator Author

albanD commented Jun 8, 2021

This should be ready for final review now!

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@albanD merged this pull request in 4512d75.

@janeyx99
Copy link
Copy Markdown
Contributor

janeyx99 commented Jun 9, 2021

Reverting as this belongs to a PR that broke some forward_mode_AD tests, e.g., https://app.circleci.com/pipelines/github/pytorch/pytorch/332766/workflows/901123bb-fbd5-48ef-b654-8e6926929c8f/jobs/13988206

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request has been reverted by 82466e0.

deniskokarev pushed a commit to deniskokarev/pytorch that referenced this pull request Jun 9, 2021
Summary: Pull Request resolved: pytorch#59448

Test Plan: Imported from OSS

Reviewed By: zou3519

Differential Revision: D28900487

Pulled By: albanD

fbshipit-source-id: e9065c5b29078d92ea9b746e188ebc1e62a407a0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants