Add per-element unique op for CPU by theweiho · Pull Request #5503 · pytorch/pytorch

theweiho · 2018-03-01T20:21:13Z

Questions/possible future works:

How to template-ize to extend support beyond LongTensor?
How to check if autograd works (and if not, how to add explicit gradient)?
CUDA support?

Testing command:
DEBUG=1 NO_CUDA=1 MACOSX_DEPLOYMENT_TARGET=10.9 CC=clang CXX=clang++ python setup.py build && DEBUG=1 NO_CUDA=1 MACOSX_DEPLOYMENT_TARGET=10.9 CC=clang CXX=clang++ python setup.py develop && python3 test/test_torch.py

Commands to preview generated documentations:
cd docs
pip install -r requirements.txt
make html

theweiho · 2018-03-01T20:30:02Z

@soumith @goldsborough @jamesr66a
#2031
onnx/onnx#568

gchanan · 2018-03-01T21:03:59Z

On your questions:

Look at the dispatch functions in Dispatch.h
You can write a test in test/test_autograd.py; if you need an explicit gradient you can add it in derivatives.yaml
You can use a dispatch: declaration in native_functions.yaml; see CUDA: examples in that file.

Sign in to view

goldsborough · 2018-03-01T21:10:40Z

Look great, had mostly nits.
For generic types, the way you'll want to do this is move all your logic into a separate template function (e.g. in an anonymous namespace above):

template<typename scalar_t>
void unique_op(at::Tensor ...) {
// use scalar_t instead of int64_t here
}

then in the current unique function, something like:

AT_DISPATCH_ALL_TYPES(self.type(), "unique", [&] {
  unique_op<scalar_>t(tensors...);
});

See here for an example.

EDIT: This is a better example for CPU: https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/TensorCompare.cpp

Sign in to view

ssnl

This at its current state still needs more work:

Documentation at _torch_docs.py and _tensor_docs.py
Dispatch to other types can be in a later PR, but this will fail with cuda long tensor (probably a segfault). Can you add a cuda version as well?

Sign in to view

goldsborough · 2018-03-02T04:16:49Z

I think you accidentally merged instead of rebasing

theweiho · 2018-03-02T04:42:59Z

Fixed merge. Documentations added and exposed unique as function as well. In order to do so, it turns out using IndexTensor in native_functions.yaml makes it so that the fn is not exposed as torch.unique(), but only as torch.autograd.Variable.unique() (thanks @gchanan!) - so changed to generic Tensor and also added dispatch to other types.

On further thought, realized that there's not really a sensible way to do gradients for this, so explicitly declared it as not implemented.

Regarding CUDA support, @ssnl - I pinged you, but it seems you're off the grid? In any case, I'd probably prefer to defer that - so is there a way to explicitly warn/catch CUDA usage for more graceful failure than segfault?

Sign in to view

+#include <unordered_map>
+#include <unordered_set>
+
+#include <iostream>


Sign in to view

+  if (sorted) {
+    std::vector<scalar_t> vec(set.begin(), set.end());
+    std::sort(vec.begin(), vec.end());
+    std::copy(vec.begin(), vec.end(), output->data<scalar_t>());


Sign in to view

+        self.assertEqual(empty_inverse, x_inverse)
+
+        x_unique, x_inverse = torch.autograd.Variable.unique(
+            x, sorted=True, return_inverse=True)


Sign in to view

+            return_inverse=True,
+        )
+        self.assertEqual(torch.ByteTensor([7, 42, 128, 133]), byte_unique)
+        self.assertEqual(torch.LongTensor([3, 0, 0, 0, 1, 2]), byte_inverse)


Sign in to view

+        - **output** (*Tensor*): the list of unique scalar elements
+        - **inverse_indices** (*Tensor*): the indices (same shape as input)
+            for where elements in the original input map to in the output
+            if ``return_inverse`` is ``True``; otherwise, an empty tensor.


fmassa · 2018-03-02T13:15:43Z

@theweiho about gradients for unique, can't we consider it's gradient as similar to indexing only the first unique elements of a tensor?

theweiho · 2018-03-02T21:31:35Z

@ssnl - is there some way to use Travis CI (or something else) to test the CUDA part of the code you suggested?

@fmassa - that was the way we were considering doing, but not sure that totally makes sense. Attributing the gradient to the first unique element seems a bit arbitrary? (Considering all occurrences of that element "contributes" to the unique equally) I also wasn't sure what a use case for the unique gradient would be, so figured it may make sense to defer it until someone has concrete requirements.

Sign in to view

+    const bool return_inverse,
+    Tensor* output,
+    Tensor* inverse_indices) {
+  set_type<scalar_t> set(


Sign in to view

+
+  if (sorted) {
+    AT_DISPATCH_ALL_TYPES(self.type(), "unique", [&] {
+      _unique_cpu_template<std::set, scalar_t>(


Sign in to view

+  throw std::runtime_error(
+      "unique is currently CPU-only, and lacks CUDA support. "
+      "Pull requests welcome!");
+  return std::make_tuple(self.type().tensor({0}), self.type().tensor({0}));


Sign in to view

+    }
+    for (int i = 0; i < input.numel(); ++i) {
+      inverse_indices.data<int64_t>()[i] =
+          inverse_map[input.data<scalar_t>()[i]];


Sign in to view

+  const Tensor& input = self.contiguous();
+  set_type<scalar_t> set(
+      input.data<scalar_t>(), input.data<scalar_t>() + input.numel());
+  Tensor output = input.type().tensor({static_cast<long long>(set.size())});


Sign in to view

+            expected_unique.tolist(), sorted(x_unique.tolist()))
+        self.assertEqual(empty_inverse, x_inverse)
+
+        x_unique, x_inverse = x.unique(return_inverse=True)


Sign in to view

+#include "ATen/ATen.h"
+
+#include <tuple>
+A


colesbury · 2018-03-06T21:13:53Z

@pytorchbot add to whitelist

ezyang · 2018-03-06T21:46:21Z

@pytorchbot retest this please

ezyang · 2018-03-06T22:32:16Z

@pytorchbot retest this please

ezyang · 2018-03-06T23:06:35Z

@pytorchbot retest this please

…o returning a 0-length tensor, per off-line reviewer comments

Sign in to view

+        x_unique, x_inverse = x.unique(return_inverse=True)
+        self.assertEqual(
+            expected_unique.tolist(), sorted(x_unique.tolist()))
+        self.assertEqual(expected_inverse.numel(), x_inverse.numel())


ezyang

I don't think the tests in the hashing case are correct.

it's testing numel, not the array

Questions/possible future works: How to template-ize to extend support beyond LongTensor? How to check if autograd works (and if not, how to add explicit gradient)? CUDA support? Testing command: DEBUG=1 NO_CUDA=1 MACOSX_DEPLOYMENT_TARGET=10.9 CC=clang CXX=clang++ python setup.py build && DEBUG=1 NO_CUDA=1 MACOSX_DEPLOYMENT_TARGET=10.9 CC=clang CXX=clang++ python setup.py develop && python3 test/test_torch.py Partially fixes pytorch#2031 * Initial commit for unique op * Working unique with test * Make inverse indices shape conform to input * flake8 whitespace removal * address review comment nits * Expose fn and add docs. Explicitly declare no gradients * Trial generic dispatch implementation * Add tests for generics * flake8 whitespace * Add basic CUDA error throwing and templateize set * Explicit contiguous and AT_DISPATCH_ALL_TYPES return * Remove extraneous numpy conversion * Refactor out .data calls * Refactored to variable return length API with wrapper fn as opposed to returning a 0-length tensor, per off-line reviewer comments * Remove A * Don't use hidden torch._unique() in test * Fix documentations

goldsborough suggested changes Mar 1, 2018

View reviewed changes

ssnl reviewed Mar 1, 2018

View reviewed changes

Comment thread aten/src/ATen/native/Unique.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ssnl requested changes Mar 1, 2018

View reviewed changes

Comment thread aten/src/ATen/native/native_functions.yaml Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

theweiho added 8 commits March 1, 2018 19:03

Initial commit for unique op

0904426

Working unique with test

a8d5ffa

Make inverse indices shape conform to input

55a8b5a

flake8 whitespace removal

a197a7b

address review comment nits

3c26309

Expose fn and add docs. Explicitly declare no gradients

b48b427

Trial generic dispatch implementation

00244e8

Add tests for generics

bd49d0a

theweiho force-pushed the unique-op branch from 84d8ed3 to bd49d0a Compare March 2, 2018 04:37

flake8 whitespace

a3dc7b2

goldsborough reviewed Mar 2, 2018

View reviewed changes

Comment thread aten/src/ATen/native/Unique.cpp Outdated

#include <unordered_map>

#include <unordered_set>

#include <iostream>

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

apaszke reviewed Mar 2, 2018

View reviewed changes

Add basic CUDA error throwing and templateize set

a016ea2

colesbury reviewed Mar 2, 2018

View reviewed changes

Explicit contiguous and AT_DISPATCH_ALL_TYPES return

294ed5d

theweiho changed the title ~~Add 1-D unique op for LongTensor~~ Add per-element unique op Mar 2, 2018

theweiho changed the title ~~Add per-element unique op~~ Add per-element unique op for CPU Mar 2, 2018

Remove extraneous numpy conversion

6534579

apaszke reviewed Mar 4, 2018

View reviewed changes

Refactor out .data calls

69c61f4

colesbury reviewed Mar 6, 2018

View reviewed changes

theweiho added 4 commits March 6, 2018 15:08

Refactored to variable return length API with wrapper fn as opposed t…

7ae734b

…o returning a 0-length tensor, per off-line reviewer comments

Remove A

b07dcc3

Don't use hidden torch._unique() in test

6c1b125

Fix documentations

eeebf16

ezyang reviewed Mar 7, 2018

View reviewed changes

ezyang previously requested changes Mar 7, 2018

View reviewed changes

ezyang merged commit c2721ab into pytorch:master Mar 7, 2018

theweiho deleted the unique-op branch March 7, 2018 23:20

ashishlal mentioned this pull request Mar 8, 2018

../libATen.so.1: undefined reference to `cublasGemmStridedBatchedEx' #5631

Closed

Conversation

theweiho commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theweiho commented Mar 1, 2018

Uh oh!

gchanan commented Mar 1, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

goldsborough commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

ssnl left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

goldsborough commented Mar 2, 2018

Uh oh!

theweiho commented Mar 2, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

fmassa commented Mar 2, 2018

Uh oh!

theweiho commented Mar 2, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

theweiho commented Mar 1, 2018 •

edited

Loading

goldsborough commented Mar 1, 2018 •

edited

Loading