Revised sparse tensor documentation. #45400

pearu · 2020-09-27T21:28:15Z

Fixes #44635.

hameerabbasi

A first review pass.

docs/source/sparse.rst

willwray

Only spotted a single sentence; suggested change in comment

test/test_torch.py

docs/source/sparse.rst

albanD

That looks very good.
Thanks for taking the time to do this.
I added small comments inline.

docs/source/sparse.rst

torch/_tensor_docs.py

docs/source/sparse.rst

torch/_tensor_docs.py

pearu · 2020-10-02T11:56:04Z

@vadimkantorov , could you also review this PR which is about updating the torch.sparse documentation page?

pearu · 2020-10-04T16:47:32Z

This PR is ready for review. Latest updates include:

add "Linear Algebra operations" section that lists operations that support sparse tensors
numerous changes based on reviewers feedback
undocument internal methods _values, _indices, _nnz and started PR Add optional coalesce argument to values(), indices(), and to a new method nse() #45695.

^ @albanD @ngimel @mruberry @ezyang @rgommers @hameerabbasi

ezyang · 2020-10-07T03:57:47Z

Therefore, I would suggest not to implement any API changes within this PR but discuss these in a separate issue/PR/RFC. What do you think?

I didn't get the sense from Mike's comments that he wanted substantive code changes in this PR, but let me just reiterate: there should be no code changes here, the docs here are As Is. That being said, if there are some weird parts of the API that we'd plan on just fixing later, we shouldn't be as nitpicky about the docs for those parts in this PR. (Not sure how much is covered by that).

mruberry · 2020-10-07T04:14:56Z

English and various notation fixes - this I can tackle according to your review immediately

Sounds good. I hope they're helpful.

overall structure of the sparse docs - it will be an iterative process that needs to be continued

Sure.

Sparse tensor API - how much of API backward compatibility we are required to keep? I think some API changes should be discussed by creating the corresponding RFCs rather than implementing these in this PR. For instance, the to_dense and to_sparse methods could be merged into one method, to or to_layout, because of the emergence of new sparse format implementations, or use another approach that includes introducing methods like to_strided, to_sparse_coo, to_sparse_gcs, etc. This PR is about updating sparse documentation according to the current state (because this is what the current users are faced to) and, when possible, take into account possible future developments. Therefore, I would suggest not to implement any API changes within this PR but discuss these in a separate issue/PR/RFC. What do you think?

I understand this PR is just updating the documentation and I agree with @ezyang that it shouldn't include code changes. The API-related questions are to help me understand the language, goals, and motivations of our sparse story so we can be consistent with our documentation. I would actually suggest this PR hew closer to the current state of the code, as mentioned previously, and maybe not try to provide an organizational structure for a new sparse format or mention the concept of a fill value as anything but zero. Following @ezyang's point, where the docs or current functions do exhibit wonky behavior (like sometimes inferring a non-zero fill value) maybe we should just address those discrepancies as straightforwardly as possible. For example, for softmax we could just add a line saying the function conceptually treats unspecified values as negative infinity, and not as zeros.

ezyang · 2020-10-12T21:27:44Z

Hey, can we agree on who is shepherding this PR? @albanD, @mruberry and I have all done substantial comments; who is going to be in charge of making sure this actually lands?

pearu · 2020-10-12T21:35:23Z

FYI, I am in a process of rewriting sparse.rst based on @mruberry feedback.

mruberry · 2020-10-12T21:44:55Z

Hey, can we agree on who is shepherding this PR? @albanD, @mruberry and I have all done substantial comments; who is going to be in charge of making sure this actually lands?

I got it.

hameerabbasi · 2020-10-15T07:33:54Z

Please feel free to re-request for review once you're done working on this.

Summary: Fixes #45113 Description: - Fixed bug in sspaddmm by calling contiguous on indices. - Added tests We have to make indices contiguous as we use `indices.data_ptr` in `_to_csr` which assumes row-contiguous storage: https://github.com/pytorch/pytorch/blob/be45c3401af8186f97f0e2b269ff3bafaf16157f/aten/src/ATen/native/sparse/SparseTensorMath.cpp#L1087-L1090 > Part 1 of fixing this is probably to document sspaddmm. Part 2 may be to rewrite it using other ops. (#45113 (comment)) - Docs will be written here: #45400 Pull Request resolved: #45963 Reviewed By: malfet Differential Revision: D24335599 Pulled By: ngimel fbshipit-source-id: 8278c73a1b4cccc5e22c6f3818dd222588c46b45

mruberry

This is a great, significant improvement to our sparse documentation. Thank you for taking the time to develop these docs, @pearu! Writing is hard and time consuming.

There are still a few grammatical issues and rough edges, but I think those are best addressed going forward.

facebook-github-bot

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

facebook-github-bot · 2020-10-22T10:10:59Z

@mruberry merged this pull request in 905ed3c.

pearu added module: sparse Related to torch.sparse module: docs Related to our documentation, both in docs/ and docblocks open source labels Sep 27, 2020

pearu self-assigned this Sep 27, 2020

pearu changed the title ~~Revise sparse tensor documentation.~~ Revised sparse tensor documentation. Sep 28, 2020

pearu force-pushed the pearu/44635 branch from 83f82fa to 17f4f3d Compare September 28, 2020 11:52

hameerabbasi reviewed Sep 28, 2020

View reviewed changes

willwray reviewed Sep 28, 2020

View reviewed changes

docs/source/sparse.rst Outdated Show resolved Hide resolved

willwray suggested changes Sep 28, 2020

View reviewed changes

rgommers reviewed Sep 28, 2020

View reviewed changes

test/test_torch.py Outdated Show resolved Hide resolved

rgommers reviewed Sep 28, 2020

View reviewed changes

docs/source/sparse.rst Outdated Show resolved Hide resolved

pearu marked this pull request as ready for review September 28, 2020 18:53

pearu requested review from albanD, ezyang, ngimel and vincentqb September 29, 2020 11:09

zhangguanheng66 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Sep 29, 2020

albanD reviewed Sep 29, 2020

View reviewed changes

docs/source/sparse.rst Outdated Show resolved Hide resolved

torch/_tensor_docs.py Outdated Show resolved Hide resolved

pearu requested a review from albanD September 30, 2020 19:44

mruberry self-requested a review September 30, 2020 21:37

pearu force-pushed the pearu/44635 branch from 1418fcd to 23e0304 Compare October 1, 2020 07:52

ezyang reviewed Oct 1, 2020

View reviewed changes

docs/source/sparse.rst Outdated Show resolved Hide resolved

ezyang reviewed Oct 1, 2020

View reviewed changes

docs/source/sparse.rst Outdated Show resolved Hide resolved

ezyang reviewed Oct 1, 2020

View reviewed changes

torch/_tensor_docs.py Outdated Show resolved Hide resolved

pearu mentioned this pull request Oct 1, 2020

Add optional coalesce argument to values(), indices(), and to a new method nse() #45695

Closed

11 tasks

pytorch deleted a comment from codecov bot Oct 2, 2020

pearu force-pushed the pearu/44635 branch 2 times, most recently from a30dee7 to 4e46790 Compare October 2, 2020 18:30

vfdev-5 mentioned this pull request Oct 7, 2020

Fixes bug in sspaddmm (#45113) #45963

Closed

This was referenced Oct 11, 2020

Deprecate spmm and dsmm functions #46164

Open

Deprecate the usage of to_dense and to_sparse methods. #46165

Closed

pearu marked this pull request as draft October 13, 2020 13:37

pearu added 8 commits October 14, 2020 12:48

Revise sparse tensor documentation.

5907555

Try undo third_party/tensorpipe changes.

8693fe9

Add bmm and mv to LA support table.

f839b6f

Apply a portion of mruberry review.

99fcc45

Update from Mike feedback.

af6a8b3

Rewrite sparse.rst [WIP]

8b341bc

Fix links and update.

47c87a8

Minor fixes

09c26bf

pearu force-pushed the pearu/44635 branch from eae12a8 to 09c26bf Compare October 14, 2020 09:49

Update

c17d40f

pearu marked this pull request as ready for review October 14, 2020 11:33

mruberry self-requested a review October 16, 2020 09:36

mruberry approved these changes Oct 16, 2020

View reviewed changes

facebook-github-bot reviewed Oct 16, 2020

View reviewed changes

facebook-github-bot closed this in 905ed3c Oct 22, 2020

facebook-github-bot added the Merged label Oct 22, 2020

pearu deleted the pearu/44635 branch October 22, 2020 15:03

Revised sparse tensor documentation. #45400

Revised sparse tensor documentation. #45400

Uh oh!

Conversation

pearu commented Sep 27, 2020

Uh oh!

hameerabbasi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willwray left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pearu commented Oct 2, 2020

Uh oh!

pearu commented Oct 4, 2020

Uh oh!

ezyang commented Oct 7, 2020

Uh oh!

mruberry commented Oct 7, 2020

Uh oh!

ezyang commented Oct 12, 2020

Uh oh!

pearu commented Oct 12, 2020

Uh oh!

mruberry commented Oct 12, 2020

Uh oh!

hameerabbasi commented Oct 15, 2020

Uh oh!

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants