Skip to content

Remove setting coalesce to 0 in sparse transpose_ #4707

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

Merged
merged 3 commits into from
Jan 24, 2018

Conversation

zou3519
Copy link
Contributor

@zou3519 zou3519 commented Jan 17, 2018

coalesced means that the sparse tensor's indices are all unique.

Let's say we transpose a sparse tensor in dimensions d0, d1. Let's say each index is some x_i = (y_1, y_2, ..., y_n) where n is the total number of dimensions of the sparse tensor. Transposing the sparse tensor in place is equivalent to swapping y_{d0}, y_{d1} for all indices x_i.

If all the indices were unique before, then they will be unique after. If they were non-unique before, they will be non-unique after.

This change removes setting coalesced to 0 after every transpose. In practice it doesn't affect very much but it's good for correctness.

Test Plan

run tests

@zou3519 zou3519 changed the title Remove setting coalesce to 0 in sparse transpose_ [wip] Remove setting coalesce to 0 in sparse transpose_ Jan 17, 2018
@zou3519 zou3519 changed the title [wip] Remove setting coalesce to 0 in sparse transpose_ Remove setting coalesce to 0 in sparse transpose_ Jan 17, 2018
@ezyang
Copy link
Contributor

ezyang commented Jan 17, 2018

Nice catch!

@soumith
Copy link
Member

soumith commented Jan 17, 2018

should you also change THCSTensor ?

@ezyang
Copy link
Contributor

ezyang commented Jan 18, 2018

@pytorchbot retest this please

@colesbury
Copy link
Member

Can you add a unit test for this behavior?

@yf225
Copy link
Contributor

yf225 commented Jan 19, 2018

@pytorchbot retest this please

1 similar comment
@yf225
Copy link
Contributor

yf225 commented Jan 19, 2018

@pytorchbot retest this please

@kose-y
Copy link

kose-y commented Apr 3, 2018

This is incorrect. indices should not only be unique, but also be sorted in coalesced matrix for correct behavior of matrix multiplication. This raises a major issue when performing matrix multiplication with coalesced then transposed matrix. See #6219.

facebook-github-bot pushed a commit that referenced this pull request Aug 15, 2018
…iants (#10496)

Summary:
- fixes #6219
- removed invariants at #4707
- assume a sparse tensor with coalesced=true when:
1. its elements are unique and
2. the indices are in sorted order
Pull Request resolved: #10496

Differential Revision: D9311214

Pulled By: weiyangfb

fbshipit-source-id: 167fa5a8e9e5f9c800db02f728a1194029f7e4f3
zdevito pushed a commit to zdevito/ATen that referenced this pull request Aug 15, 2018
…iants (#10496)

Summary:
- fixes pytorch/pytorch#6219
- removed invariants at pytorch/pytorch#4707
- assume a sparse tensor with coalesced=true when:
1. its elements are unique and
2. the indices are in sorted order
Pull Request resolved: pytorch/pytorch#10496

Differential Revision: D9311214

Pulled By: weiyangfb

fbshipit-source-id: 167fa5a8e9e5f9c800db02f728a1194029f7e4f3
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
…iants (pytorch#10496)

Summary:
- fixes pytorch#6219
- removed invariants at pytorch#4707
- assume a sparse tensor with coalesced=true when:
1. its elements are unique and
2. the indices are in sorted order
Pull Request resolved: pytorch#10496

Differential Revision: D9311214

Pulled By: weiyangfb

fbshipit-source-id: 167fa5a8e9e5f9c800db02f728a1194029f7e4f3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants