Skip to content

Add empty tensor tests to test_sparse#11228

Closed
yf225 wants to merge 10 commits intopytorch:masterfrom
yf225:sparse_op_test_fixes
Closed

Add empty tensor tests to test_sparse#11228
yf225 wants to merge 10 commits intopytorch:masterfrom
yf225:sparse_op_test_fixes

Conversation

@yf225
Copy link
Contributor

@yf225 yf225 commented Sep 4, 2018 •
edited
Loading

This PR adds empty sparse tensor tests to test_sparse.py, and also fix various places in internal code to make the tests pass.

@yf225 yf225 requested a review from gchanan September 4, 2018 17:45
@yf225 yf225 force-pushed the sparse_op_test_fixes branch 3 times, most recently from dc10d93 to d452320 Compare September 7, 2018 17:37
facebook-github-bot
facebook-github-bot reviewed Sep 10, 2018
View reviewed changes
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.

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

@yf225 yf225 force-pushed the sparse_op_test_fixes branch from c6fd8f3 to 33e2682 Compare September 11, 2018 17:07
facebook-github-bot
facebook-github-bot reviewed Sep 11, 2018
View reviewed changes
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.

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

gchanan
gchanan reviewed Sep 11, 2018
View reviewed changes
aten/src/ATen/native/sparse/SparseTensor.cpp Outdated

This comment was marked as off-topic.

Sign in to view

aten/src/ATen/native/sparse/SparseTensor.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

test/test_sparse.py Outdated

This comment was marked as off-topic.

Sign in to view

@yf225 yf225 changed the title Add empty tensor tests to test_sparse [API CHANGE] Add empty tensor tests to test_sparse Sep 12, 2018
ssnl
ssnl reviewed Sep 13, 2018
View reviewed changes
aten/src/ATen/native/sparse/SparseTensor.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@gchanan
Copy link
Contributor

gchanan commented Sep 13, 2018

don't you want to do the above in cuda as well?

@yf225 yf225 force-pushed the sparse_op_test_fixes branch 2 times, most recently from 7d2485f to 3ec5014 Compare September 13, 2018 20:27
@yf225
Copy link
Contributor Author

yf225 commented Sep 13, 2018 •
edited
Loading

@gchanan yes sorry for oversight - just added it to cuda as well

@yf225
Copy link
Contributor Author

yf225 commented Sep 13, 2018

@pytorchbot retest this please

2 similar comments
@yf225
Copy link
Contributor Author

yf225 commented Sep 13, 2018

@pytorchbot retest this please

@yf225
Copy link
Contributor Author

yf225 commented Sep 13, 2018

@pytorchbot retest this please

@yf225 yf225 force-pushed the sparse_op_test_fixes branch 2 times, most recently from 3681f69 to 8d7f6cf Compare September 17, 2018 22:56
gchanan
gchanan approved these changes Sep 17, 2018
View reviewed changes
aten/src/ATen/native/sparse/cuda/SparseCUDATensor.cu Outdated

This comment was marked as off-topic.

Sign in to view

aten/src/ATen/native/sparse/cuda/SparseCUDATensor.cu Outdated

This comment was marked as off-topic.

Sign in to view

aten/src/ATen/native/sparse/SparseTensor.cpp Outdated

This comment was marked as off-topic.

Sign in to view

facebook-github-bot
facebook-github-bot reviewed Sep 18, 2018
View reviewed changes
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.

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

@ssnl
Copy link
Collaborator

ssnl commented Sep 18, 2018

yay. I'll rebase #11232 on top of master.

@yf225 yf225 reopened this Sep 18, 2018
@yf225
Copy link
Contributor Author

yf225 commented Sep 18, 2018

Unlanding this PR because of a test error on master: https://ci.pytorch.org/jenkins/job/pytorch-builds/job/pytorch-linux-xenial-cuda8-cudnn6-py3-test2/10441//console. Will investigate using the CI in this PR.

@yf225 yf225 force-pushed the sparse_op_test_fixes branch from 5a2f586 to af8a22c Compare September 18, 2018 18:18
@yf225 yf225 reopened this Sep 18, 2018
@yf225 yf225 force-pushed the sparse_op_test_fixes branch from 179fba8 to fe5f89c Compare September 18, 2018 23:07
facebook-github-bot
facebook-github-bot reviewed Sep 19, 2018
View reviewed changes
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.

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

@yf225 yf225 changed the title [API CHANGE] Add empty tensor tests to test_sparse Add empty tensor tests to test_sparse Sep 19, 2018
zdevito pushed a commit to zdevito/ATen that referenced this pull request Sep 19, 2018
@facebook-github-bot
Add empty tensor tests to test_sparse (#11228)
524ce91 
Summary:
This PR adds empty sparse tensor tests to `test_sparse.py`, and also fix various places in internal code to make the tests pass.

**[NOTE] API CHANGE:**
- `coalesce` on sparse tensor will always be performed out-of-place now (meaning the original tensor will never be affected)
Pull Request resolved: pytorch/pytorch#11228

Differential Revision: D9930449

Pulled By: yf225

fbshipit-source-id: 7c62439b216a6badf7938a10741c358ff18a556d
@ezyang ezyang added the merged label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ssnl ssnl ssnl left review comments

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@ezyang ezyang ezyang approved these changes

@gchanan gchanan gchanan approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@colesbury colesbury Awaiting requested review from colesbury

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yf225 @gchanan @ssnl @ezyang @facebook-github-bot