-
Notifications
You must be signed in to change notification settings - Fork 24.7k
add support for sparse tensors in torch.testing.assert_close
#58844
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
Conversation
💊 CI failures summary and remediationsAs of commit fc903d5 (more details on the Dr. CI page and at hud.pytorch.org/pr/58844):
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. |
Codecov Report
@@ Coverage Diff @@
## master #58844 +/- ##
=======================================
Coverage 76.23% 76.24%
=======================================
Files 2054 2054
Lines 205033 205075 +42
=======================================
+ Hits 156309 156350 +41
- Misses 48724 48725 +1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some comments and made suggestions regarding testing uncoalesced sparse tensors.
The PR should implement support for sparse csr tensors as well. As a hint, sparse and sparse csr tensors have the following differences:
- sparse has two strided members (
indices
andvalues
) while sparse csr has three:crow_indices
,col_indices
m andvalues
- sparse csr is always a 2D tensor while sparse represents a ND tensor
- the coalesce concept exists only for sparse, not for sparse csr. Hence,
_indices
and_values
methods are defined only for sparse layout.
In all other aspects, sparse and sparse csr are similar.
…torch into testing-asserts-sparse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pearu Do you think we should add an example for sparse comparisons to our docstring? If yes, for what use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I have only some nits regarding CSR documentation bit and enforcing equality checks for COO and CSR indices. Thanks, @pmeier !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the concern about what happens when a new sparse format is introduced, is real.
Currently, the checks may give false-positive results when, say, one of the operands (actual
or expected
) is a strided tensor and another is an instance of a new sparse format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I am uncertain if we should use "uncoalesced" or "non-coalesced".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
This hit some internal test failures on the pytorch_core-buck job which appear relevant: caffe2/test:testing - test_mismatching_values_msg (test_testing.TestAssertsSparseCSR)
Looks like that job isn't built with sparse support; fyi @cpuhrsch One simple option to unblock this PR is to decorate tests using the sparse CSR format with
I'm not sure how test_sparse_csr.py isn't run in this build, but it appears to be filtered somewhere? cc @pearu - any ideas? |
@mruberry can you share the cmake "Summary" part from the build log? Perhaps this could provide hints how to reproduce this externally.. |
It's a buck build so there's a lot of buck targets to scrape through for how this is actually built, but it looks like this test is being filtered directly by the test command, which is opt-in:
|
@mruberry , should |
test_testing.py seems like the right place for it. Yes, the COO-related tests appear to be passing:
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
This adds support for sparse tensors the same way
torch.testing._internal.common_utils.TestCase.assertEqual
does:pytorch/torch/testing/_internal/common_utils.py
Lines 1287 to 1313 in 5c7dace