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

sparse.mm.backward: fix for non-contiguous grad values on CPU #106127

Closed
wants to merge 3 commits into from

Conversation

nikitaved
Copy link
Collaborator

@nikitaved nikitaved commented Jul 27, 2023

Fixes #102493.
The problem was that the backward implementation assumed inputs to be contiguous.
This might supersede #104520.

Stack from ghstack (oldest at bottom):

cc @alexsamardzic @pearu @cpuhrsch @amjames @bhosmer @jgong5 @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10

@pytorch-bot
Copy link

pytorch-bot bot commented Jul 27, 2023

🔗 Helpful Links

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

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

✅ No Failures

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

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

@pytorch-bot pytorch-bot bot added the release notes: sparse release notes category label Jul 27, 2023
nikitaved added a commit that referenced this pull request Jul 27, 2023
ghstack-source-id: bfd5b612a65f91ef50a03bf35ad9d87aa8f9f3fc
Pull Request resolved: #106127
nikitaved added a commit that referenced this pull request Jul 27, 2023
ghstack-source-id: 0763a7a1591633bb1f0af595610887b9070b9f9c
Pull Request resolved: #106127
@nikitaved nikitaved added the ciflow/trunk Trigger trunk jobs on your pull request label Jul 27, 2023
Comment on lines 109 to 113
typename index_t_ptr::value_type Cp[],
// NOLINTNEXTLINE(modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays)
int64_t Cj[],
typename index_t_ptr::value_type Cj[],
// NOLINTNEXTLINE(modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays)
scalar_t Cx[]) {
typename scalar_t_ptr::value_type Cx[]) {
Copy link
Collaborator Author

@nikitaved nikitaved Jul 27, 2023

Choose a reason for hiding this comment

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

Let's probably not assume that C-arrays are always contiguous...
EDIT: not needed. This function is used in a single place, and only receives contiguous output.

…CPU"

Fixes #102493.
The problem was that the backward implementation assumed inputs to be contiguous.
This might supersede #104520. 




[ghstack-poisoned]
nikitaved added a commit that referenced this pull request Jul 27, 2023
ghstack-source-id: aaf13ecfaef31d33b6ff858985e692f3fc392907
Pull Request resolved: #106127
@nikitaved nikitaved added module: sparse Related to torch.sparse module: cpu CPU specific problem (e.g., perf, algorithm) labels Jul 27, 2023
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.

Thank you @nikitaved!

@nikitaved
Copy link
Collaborator Author

@pytorchbot merge -i

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged while ignoring the following 0 checks:

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

bobby-palmer pushed a commit to bobby-palmer/pytorch that referenced this pull request Jul 29, 2023
…h#106127)

Fixes pytorch#102493.
The problem was that the backward implementation assumed inputs to be contiguous.
This might supersede pytorch#104520.

Pull Request resolved: pytorch#106127
Approved by: https://github.com/cpuhrsch
@facebook-github-bot facebook-github-bot deleted the gh/nikitaved/64/head branch July 31, 2023 14:17
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: cpu CPU specific problem (e.g., perf, algorithm) 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

4 participants