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

enable backward for log1p (sparse layouts) #88155

Closed
wants to merge 4 commits into from

Conversation

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 31, 2022

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/88155

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 274846a:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

test/test_sparse.py Outdated Show resolved Hide resolved
@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 1, 2022
Copy link
Contributor

@cpuhrsch cpuhrsch left a comment

Choose a reason for hiding this comment

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

See comments.

cc nikitaved pearu cpuhrsch bhosmer

[ghstack-poisoned]
amjames added a commit that referenced this pull request Nov 1, 2022
ghstack-source-id: 93d98e44b99c40083844274263f8c79c36a6779c
Pull Request resolved: #88155
cc nikitaved pearu cpuhrsch bhosmer

[ghstack-poisoned]
amjames added a commit that referenced this pull request Nov 2, 2022
ghstack-source-id: 13be4fa066994ba23aa7a72e9e207b58762eeb67
Pull Request resolved: #88155
@amjames
Copy link
Collaborator Author

amjames commented Nov 2, 2022

@cpuhrsch this is ready for another look when you get a chance

Comment on lines +4857 to +4860
// If grad is sparse we can't divide by the n-d (self + 1).conj(), so we
// must multiply by the recipricol, layout of grad is preserved which is
// important to gradcheck
return grad * self_p1_conj.reciprocal_();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is that? Because it is not supported?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, div supports scalar or zero-dim dense tensor divisors only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see. We can have it enabled for COO at least now that mul is merged :)

cc nikitaved pearu cpuhrsch bhosmer

[ghstack-poisoned]
@amjames
Copy link
Collaborator Author

amjames commented Nov 4, 2022

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Nov 5, 2022
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Dec 10, 2022
@facebook-github-bot facebook-github-bot deleted the gh/amjames/21/head branch June 8, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged module: sparse Related to torch.sparse open source release notes: sparse release notes category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants