Dense <-> bsc conversions #80781

amjames · 2022-07-01T19:23:45Z

Stack from ghstack (oldest at bottom):

[ghstack-poisoned]

ghstack-source-id: 9a9e68b Pull Request resolved: #80781

facebook-github-bot · 2022-07-01T19:23:53Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/80781
✖️ Python docs build was skipped
✖️ C++ docs build was skipped
❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (3 Pending)

As of commit 925ca12 (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

amjames · 2022-07-01T19:27:01Z

An issue that will probably need some discussion. It appears that SciPy does not implement the BSC format, so the existing testing framework is not going to work out of the box. We could do some combination of checking indices with a compatible CSC tensor and manually checking block-by-block value equality, but I'm all for a better solution if there is one on offer.

cc @cpuhrsch @pearu

aten/src/ATen/native/TensorConversions.cpp

cpuhrsch · 2022-07-01T22:44:54Z

@amjames - I'm am wondering whether support for transpose(BSR) -> BSC and transpose(BSC) -> BSR could help the tests.

[ghstack-poisoned]

ghstack-source-id: 3dd050a Pull Request resolved: #80781

[ghstack-poisoned]

ghstack-source-id: 7be0686 Pull Request resolved: #80781

[ghstack-poisoned]

ghstack-source-id: 7f91f4e Pull Request resolved: #80781

aten/src/ATen/native/TensorConversions.cpp

aten/src/ATen/native/sparse/cuda/SparseCsrTensorMath.cu

[ghstack-poisoned]

nikitaved · 2022-08-25T12:28:55Z

aten/src/ATen/native/TensorConversions.cpp

+  auto batched_out =
+      at::zeros({n_batch, n_compressed_per_batch + 1}, compressed_in.options());


need to change to compressed_in.options().dtype(out_type), then we can remove to right before return from the function.

aten/src/ATen/native/TensorConversions.cpp

nikitaved · 2022-08-25T12:40:29Z

aten/src/ATen/native/TensorConversions.cpp

+    auto offsets = compressed_in.index({Slice(0, -1, n_compressed_per_batch)})
+                       .reshape({n_batch, -1});


same here, we can use slice and reduce dependency on at::indexing.

aten/src/ATen/native/TensorConversions.cpp

[ghstack-poisoned]

amjames · 2022-08-25T22:42:19Z

aten/src/ATen/native/TensorConversions.cpp

+    auto offsets = compressed_in.slice(0, 0, -1, n_compressed_per_batch)
+                       .reshape({n_batch, -1});
+    batched_out.narrow(-1, 1, n_compressed_per_batch)
+        .copy_(trailing_slice - offsets);


@nikitaved I sort of minced your suggestions together with what was already here.

Yes, it could go in a single statement, but it is very difficult to parse and the two intermediate variables trailing_slice and offsets make it much clearer.

And you decided against in-placeness, right? I like these variables. Maybe we could also sprinkle some comments here and there...

Sorry, yes. The copy_ is fine however we can't do the subtraction in place as both factors are views on the same storage, and that sub does in fact broadcast (n_batch x n_compressed_per_batch) - (n_batch x 1)

Yeah, missed that. Good catch!

Alternatively, we could probably copy first, then do an in-place diff ?

nikitaved

Thank you, @amjames , this looks great!

[ghstack-poisoned]

amjames · 2022-09-01T15:59:29Z

@pytorchbot merge

pytorchmergebot · 2022-09-01T16:01:55Z

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the land checks (-l) flag. If you did not specify this flag yourself, you are likely enrolled in the land checks rollout. This means that your change will be merged once all checks on your PR have passed since you have added the ciflow/trunk label to your PR (ETA 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

Pull Request resolved: #80781 Approved by: https://github.com/bhosmer, https://github.com/nikitaved

github-actions · 2022-09-01T16:02:51Z

Hey @amjames.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

Summary: Pull Request resolved: #80781 Approved by: https://github.com/bhosmer, https://github.com/nikitaved Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/8778f337442ab7ad512d20c3a9028df59380c6f0 Reviewed By: mehtanirav Differential Revision: D39213811 fbshipit-source-id: 90a50de4bf00fc7294e3490c34d75264a036a364

Dense <-> bsc conversions

4d34280

[ghstack-poisoned]

amjames requested a review from bdhirsh as a code owner July 1, 2022 19:23

amjames added a commit that referenced this pull request Jul 1, 2022

Dense <-> bsc conversions

bc687dc

ghstack-source-id: 9a9e68b Pull Request resolved: #80781

facebook-github-bot added the cla signed label Jul 1, 2022

amjames marked this pull request as draft July 1, 2022 19:24

amjames added the module: sparse Related to torch.sparse label Jul 1, 2022

amjames commented Jul 1, 2022

View reviewed changes

aten/src/ATen/native/TensorConversions.cpp Outdated Show resolved Hide resolved

pytorchbot added the open source label Jul 1, 2022

Update on "Dense <-> bsc conversions"

ef61a0f

[ghstack-poisoned]

This was referenced Jul 25, 2022

Add support for select of batch dims for all sparse compressed formats. #82119

Closed

Fix BSR->Dense Batched Bug #82120

Closed

Support BSC->CSC conversion #82121

Closed

Sparse Compressed Transpose add support for Batch dims and BSR/BSC layouts #82122

Closed

amjames added a commit that referenced this pull request Jul 25, 2022

Dense <-> bsc conversions

19c934f

ghstack-source-id: 3dd050a Pull Request resolved: #80781

cpuhrsch requested a review from bhosmer July 25, 2022 16:51

Update on "Dense <-> bsc conversions"

82d4ccc

[ghstack-poisoned]

Update on "Dense <-> bsc conversions"

faa98be

[ghstack-poisoned]

Update on "Dense <-> bsc conversions"

0dcfe81

[ghstack-poisoned]

amjames added a commit that referenced this pull request Jul 25, 2022

Dense <-> bsc conversions

bfc50d7

ghstack-source-id: 7be0686 Pull Request resolved: #80781

Update on "Dense <-> bsc conversions"

1925620

[ghstack-poisoned]

Update on "Dense <-> bsc conversions"

29b2079

[ghstack-poisoned]

Update on "Dense <-> bsc conversions"

26159ca

[ghstack-poisoned]

amjames added a commit that referenced this pull request Jul 26, 2022

Dense <-> bsc conversions

2446f16

ghstack-source-id: 7f91f4e Pull Request resolved: #80781

nikitaved reviewed Aug 18, 2022

View reviewed changes

aten/src/ATen/native/TensorConversions.cpp Outdated Show resolved Hide resolved

nikitaved reviewed Aug 18, 2022

View reviewed changes

aten/src/ATen/native/TensorConversions.cpp Outdated Show resolved Hide resolved

nikitaved reviewed Aug 18, 2022

View reviewed changes

aten/src/ATen/native/sparse/cuda/SparseCsrTensorMath.cu Outdated Show resolved Hide resolved

amjames added 2 commits August 19, 2022 10:30

Update on "Dense <-> bsc conversions"

f4bd7e4

[ghstack-poisoned]

Update on "Dense <-> bsc conversions"

1798687

[ghstack-poisoned]

nikitaved reviewed Aug 25, 2022

View reviewed changes

aten/src/ATen/native/TensorConversions.cpp Show resolved Hide resolved

nikitaved reviewed Aug 25, 2022

View reviewed changes

aten/src/ATen/native/TensorConversions.cpp Outdated Show resolved Hide resolved

nikitaved reviewed Aug 25, 2022

View reviewed changes

aten/src/ATen/native/TensorConversions.cpp Outdated Show resolved Hide resolved

nikitaved reviewed Aug 25, 2022

View reviewed changes

aten/src/ATen/native/TensorConversions.cpp Outdated Show resolved Hide resolved

nikitaved reviewed Aug 25, 2022

View reviewed changes

aten/src/ATen/native/TensorConversions.cpp Show resolved Hide resolved

nikitaved reviewed Aug 25, 2022

View reviewed changes

aten/src/ATen/native/TensorConversions.cpp Show resolved Hide resolved

Update on "Dense <-> bsc conversions"

9dd3c97

[ghstack-poisoned]

amjames commented Aug 25, 2022

View reviewed changes

nikitaved approved these changes Aug 26, 2022

View reviewed changes

amjames added 3 commits August 26, 2022 11:29

Update on "Dense <-> bsc conversions"

eedd37a

[ghstack-poisoned]

Update on "Dense <-> bsc conversions"

6962cbe

[ghstack-poisoned]

Update on "Dense <-> bsc conversions"

925ca12

[ghstack-poisoned]

pytorchmergebot pushed a commit that referenced this pull request Sep 1, 2022

Dense <-> bsc conversions (#80781)

81894f7

Pull Request resolved: #80781 Approved by: https://github.com/bhosmer, https://github.com/nikitaved

pytorchmergebot added the Merged label Sep 1, 2022

pytorchmergebot closed this in 8778f33 Sep 1, 2022

amjames added release notes: sparse release notes category topic: new features topic category labels Sep 1, 2022

facebook-github-bot deleted the gh/amjames/7/head branch September 5, 2022 14:19

		auto batched_out =
		at::zeros({n_batch, n_compressed_per_batch + 1}, compressed_in.options());

		auto offsets = compressed_in.index({Slice(0, -1, n_compressed_per_batch)})
		.reshape({n_batch, -1});

Dense <-> bsc conversions #80781

Dense <-> bsc conversions #80781

Uh oh!

Conversation

amjames commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

✅ No Failures (3 Pending)

Uh oh!

amjames commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cpuhrsch commented Jul 1, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikitaved Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikitaved Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amjames Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

nikitaved Aug 26, 2022 • 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!

amjames Aug 26, 2022 • 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!

nikitaved Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

nikitaved Aug 26, 2022 • 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!

nikitaved left a comment

Choose a reason for hiding this comment

Uh oh!

amjames commented Sep 1, 2022

Uh oh!

pytorchmergebot commented Sep 1, 2022

Uh oh!

github-actions bot commented Sep 1, 2022

Uh oh!

Uh oh!

amjames commented Jul 1, 2022 •

edited

Loading

facebook-github-bot commented Jul 1, 2022 •

edited

Loading

amjames commented Jul 1, 2022 •

edited

Loading

nikitaved Aug 26, 2022 •

edited

Loading

amjames Aug 26, 2022 •

edited

Loading

nikitaved Aug 26, 2022 •

edited

Loading