Add autograd for to_sparse. #20458

nairbv · 2019-05-13T23:17:02Z

#18111

Gets TestJitGeneratedAutograd.test_to_sparse to pass but need feedbak from jit team on test_sparse_tensors_error.

nairbv · 2019-05-14T21:16:04Z

As discussed with @eellison, need feedback from jit team on support for sparse tensors.

nairbv · 2019-05-15T20:14:07Z

@pytorchbot rebase this please

eellison · 2019-05-16T16:44:34Z

I don't think we want to add the additional complexity of Sparse tensors to JIT yet.

We have an existing pass inpace_check.cpp that checks for one particular unsupported op before running the JIT. Maybe you could rename the pass and check for aten::to_sparse in that pass as well?

How does that sound @suo @zdevito

torch/csrc/jit/ir.h

torch/csrc/jit/pybind_utils.h

…nto sparse_backward

nairbv · 2019-05-21T18:13:34Z

@pytorchbot rebase this please

zou3519

logic looks fine, some comments though

torch/autograd/gradcheck.py

zou3519 · 2019-06-05T15:00:18Z

tools/autograd/derivatives.yaml

 - name: sparse_mask(Tensor self, Tensor mask)
-  self: not_implemented("sparse_mask")
-  mask: not_implemented("sparse_mask")
+  self: grad.to_dense().sparse_mask(mask).to_dense()


Why is this necessary to implement to_sparse backward?
Also, this (sparse_mask backward) needs a test given that you've implemented it.

in ./aten/src/ATen/native/TensorConversions.cpp you can see that to_dense_backward calls sparse_mask, which ends up causing this to be required.

why do you need to densify the grad? I guess we don't have a kernel doing what you want, but it seems feasible, though maybe not worth the effort.

sparse_mask requires a dense tensor and sparse mask. if I don't call to_dense() on the grad, iirc, sparse mask rejects it. If I don't call to_dense on the result, IIRC I got an error that the gradient layout has to match. I guess we could write an explicit kernel that took a sparse gradient and returned a dense mask though. I can add one, or at least file a ticket if you think it's worthwhile?

test/test_jit.py

zou3519 · 2019-06-05T15:07:26Z

test/test_autograd.py

    def test_sparse_gather_both_scalar(self):
        self._test_sparse_gather((), (), 0)

+    def test_to_sparse_autograd(self):


Why is this function necessary? There's already a to_sparse test in common_method_invocations that autograd should call

probably not, I added this first before realizing I should add in common method invocs.

I wasn't able to add a sparse mask test in common invocs because it's not setup to handle tests with sparse inputs, so I converted this test to test sparse_mask.

does sparse mask also gets tested indirectly via the common method invocations entry for to_sparse?

gradcheck and gradgradcheck are run via the common method invocations, so yes sparse_mask is indirectly tested (in the gradgradcheck step), but it's always good to have an explicit test

zou3519 · 2019-06-06T14:08:28Z

test/test_autograd.py

+    def test_sparse_mask_autograd(self):
+        for device in ['cpu', 'cuda'] if torch.cuda.is_available() else ['cpu']:
+            tensor = torch.randn(3, requires_grad=True, device=device)
+            mask = torch.ones(3, device=device).to_sparse()


You should test a mask that is not just all ones, something like torch.tensor([1, 0, 1])

ah yes, that was a bit lazy of me. fixed.

zou3519

lgtm

facebook-github-bot

@nairbv is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

gchanan · 2019-06-06T22:47:25Z

test/test_autograd.py

+            mask = mask.to_sparse()
+            converted = tensor.sparse_mask(mask).to_dense()
+            converted.sum().backward()
+            self.assertEqual(tensor.grad, mask.to_dense())


does gradcheck / gradgradcheck not work here?

I think @nairbv said gradcheck / gradgradcheck can't handle sparse tensors (as autograd leafs).

But it could be good to run gradcheck on tensor.sparse_mask(mask.to_sparse()) (where tensor, mask are dense tensors, if that works.

ya, I think we've run into this before and it can be worked around.

sparse_mask didn't allow for a dense mask

facebook-github-bot · 2019-06-07T02:37:55Z

@nairbv merged this pull request in 8a9ea55.

nairbv added 2 commits May 13, 2019 17:41

initial not-working attempt to add autograd for to_sparse

727e562

fix autogradient and tests for sparse_mask

e8b06eb

pytorchbot added module: autograd Related to torch.autograd, and the autograd engine in general module: internals Related to internal abstractions in c10 and ATen module: tests Issues related to tests (not the torch.testing module) labels May 13, 2019

Make jit tests pass for to_sparse autograd

70e8b2a

Gets TestJitGeneratedAutograd.test_to_sparse to pass but need feedbak from jit team on test_sparse_tensors_error.

pytorchbot added oncall: jit Add this issue/PR to JIT oncall triage queue module: pybind Related to our Python bindings / interactions with other Python libraries labels May 14, 2019

nairbv requested a review from eellison May 14, 2019 21:15

nairbv changed the title ~~initial not-working attempt to add autograd for to_sparse~~ Add autograd for to_sparse. May 14, 2019

pytorchbot and others added 2 commits May 15, 2019 20:14

Merge remote-tracking branch 'origin/master' into HEAD

d157c0a

remove comments

1acd129

eellison requested review from apaszke and zdevito May 16, 2019 18:07

zdevito reviewed May 17, 2019

View reviewed changes

torch/csrc/jit/ir.h Outdated Show resolved Hide resolved

torch/csrc/jit/pybind_utils.h Outdated Show resolved Hide resolved

torch/csrc/jit/pybind_utils.h Show resolved Hide resolved

nairbv added 2 commits May 21, 2019 12:27

revert jit changes and skip jit test for to_sparse

411b4f7

Merge branch 'sparse_backward' of https://github.com/nairbv/pytorch i…

cd5f545

…nto sparse_backward

pytorchbot and others added 3 commits May 21, 2019 18:13

Merge remote-tracking branch 'origin/master' into HEAD

672ad0b

Merge branch 'master' into sparse_backward

f924cd6

fix merge issues from removal of SparseTensorRef

0fb6575

nairbv requested a review from gchanan June 5, 2019 14:31

zou3519 reviewed Jun 5, 2019

View reviewed changes

some changes to to_sparse from code review

b02bc7d

zou3519 reviewed Jun 6, 2019

View reviewed changes

improve sparse mask gradient test

9322538

zou3519 approved these changes Jun 6, 2019

View reviewed changes

facebook-github-bot reviewed Jun 6, 2019

View reviewed changes

facebook-github-bot closed this in 8a9ea55 Jun 6, 2019

gchanan reviewed Jun 6, 2019

View reviewed changes

facebook-github-bot added the merged label Jun 7, 2019

nairbv mentioned this pull request Jun 13, 2019

RuntimeError: derivative for to_sparse is not implemented #18111

Closed

LiuXiaoxuanPKU mentioned this pull request Oct 22, 2020

autograd for to_sparse(dim) #46720

Closed

mruberry added the Merged label Oct 28, 2020

Add autograd for to_sparse. #20458

Add autograd for to_sparse. #20458

Uh oh!

Conversation

nairbv commented May 13, 2019

Uh oh!

nairbv commented May 14, 2019

Uh oh!

nairbv commented May 15, 2019

Uh oh!

eellison commented May 16, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nairbv commented May 21, 2019

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchanan Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zou3519 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

gchanan Jun 6, 2019 •

edited

Loading