Add comparison tensor operations #386

ViliamVadocz · 2023-01-21T18:53:54Z

Works towards #306.
Depends on coreylowman/cudarc#60.

I am creating this as a draft to get feedback on the structure before implementing other operations (although this should be quite straightforward now).

Let me know what you think.
I will add documentation once implementation is finalized.

ViliamVadocz · 2023-01-21T19:01:55Z

Some things to consider and choices I would like feedback on:

Should Unit require PartialEq? At the moment I enforce that at the implementation for EqKernelOp as an additional requirement.
I used a generic to specify the type of operation. I know that other kernel operations (such as UnaryKernel and BinaryKernel) pass in an empty struct. Should this also be done for comparison operations?
I placed the comparison operations in a sub-directory cmp I did this so that I could separate the comparison-specific kernel support code from the code for other operations which require Dtype. I think this is a good solution but perhaps a different structure is preferred?

nkoppel · 2023-01-21T20:03:23Z

I used a generic to specify the type of operation. I know that other kernel operations (such as UnaryKernel and BinaryKernel) pass in an empty struct. Should this also be done for comparison operations?

It should be fine not to. I added these struct parameters to the binary operations in #346 only because of huber_error's delta parameter, and comparison operations should not have this kind of parameter.

You should probably implement unary versions of these operations, because this is consistent with the rest of the api and will probably be the more common use case.

While tensors of booleans are currently useless with the rest of the api, I think it is best to keep the output as tensors of booleans and to create operations for them. These could include inversion (!), and (&), or (|), xor (^), and masking operations for use with other tensors.

ViliamVadocz · 2023-01-21T22:51:18Z

You should probably implement unary versions of these operations, because this is consistent with the rest of the api and will probably be the more common use case.

I don't understand how comparison could be unary. Did you mean something like comparing to a scalar? Or perhaps you meant that the comparison functions should be methods on the tensor type? (They are.)

nkoppel · 2023-01-22T04:50:47Z

Sorry for the confusion, I meant that you should add operations that act like ScalarAdd, comparing each value within the tensor with a single number, like <tensor>.eq(0.0).

coreylowman

Looks like a great start! I like the addition of CmpKernel/CmpOpCpuKernel/CmpOpCudaKernel for reducing code. Since you added the above and the actual kernel implementations look pretty small, I think combining them into the same file so the structure is the same as the other ops should be good:

- tensor_ops/
    - cmp/
        - mod.rs
        - cuda_kernels.rs
        - cpu_kernels.rs
        - cmp.cu

where they would contain the following:

mod.rs - all the tensor methods/functions & tests
cpu_kernels.rs
1. The trait definition for CmpOpCpuKernel
2. The trait impl for impl<Op: CmpOpCpuKernel<E>, E: Unit> CmpKernel<Op, E> for Cpu {
3. all the impl CmpOpCpuKernel<E> for EqKernelOp for all the ops (Eq/Lt, etc)
cuda_kernels.rs - similar to cpu kernels but for the cuda stuff
cmp.cu all the forward functions for each op

src/tensor_ops/cmp/eq/cpu_kernel.rs

src/tensor_ops/cmp/eq/mod.rs

src/shapes/shape.rs

ViliamVadocz · 2023-01-30T02:40:19Z

I added the rest of the comparison operations as well as scalar operations. I was unsure about what to name the scalar comparisons, but I went with scalar_op where op is replaced by one of eq, ne, ge, etc.

I was wondering whether I should also implement PartialEq and PartialCmp for Tensor so that it would be possible to use the actual operators like <, etc.

nkoppel · 2023-01-30T03:18:17Z

The <, ==, and other comparison operations can, unfortunately, only output single booleans. However, I think it's a good idea to implement PartialEq and PartialOrd for tensors. The cuda implementation for PartialEq should boil down to a reduction, but the kernel for ParitalOrd will need to prioritize earlier elements in the tensors, which will require something like a custom atomic operation.

coreylowman

This looks great! Structure is perfect, and a lot of good stuff in here. Only a couple things to add and then its good to go 🚀

src/tensor_ops/cmp/mod.rs

ViliamVadocz · 2023-02-03T16:21:44Z

Please note that the CUDA code relies on bool implementing ValidAsZeroBits. This was already merged in cudarc (coreylowman/cudarc#60), but a new version of the crate has not been published yet.

If you try compiling this branch with test-cuda you will get errors unless you also change the cudarc dependency to use the main branch from git. Probably best to wait with merging this until a new version of cudarc is published.

coreylowman · 2023-02-06T19:05:13Z

src/tensor_ops/cmp/mod.rs

    }
+
+    #[test]
+    #[should_panic]


Nice test 😀

coreylowman

Will merge once cudarc releases support for bools

coreylowman · 2023-02-09T13:42:34Z

Thanks for the contribution!

ViliamVadocz added 3 commits January 21, 2023 19:46

Add support for implementing comparison operations

67acec8

Implement Unit for integer types

368376a

Add eq comparison operation

8c6fb02

coreylowman reviewed Jan 22, 2023

View reviewed changes

src/tensor_ops/cmp/eq/cpu_kernel.rs Outdated Show resolved Hide resolved

coreylowman reviewed Jan 22, 2023

View reviewed changes

src/tensor_ops/cmp/eq/mod.rs Outdated Show resolved Hide resolved

coreylowman reviewed Jan 22, 2023

View reviewed changes

src/shapes/shape.rs Outdated Show resolved Hide resolved

ViliamVadocz added 8 commits January 26, 2023 21:02

Merge branch 'main' into comparison-ops

ffcf15b

Add PartialEq to Unit

98b1981

Refactor file structure for comparison operations

eba9e9c

Use references as inputs instead of owned types

f5edf2c

Add the rest of the comparison operations

4952cf3

Add scalar comparison operations

edf292f

Reduce boilerplate

261a929

Add documentation for comparison operations

7b2a81b

ViliamVadocz marked this pull request as ready for review January 30, 2023 02:35

coreylowman self-requested a review January 31, 2023 14:05

coreylowman requested changes Feb 1, 2023

View reviewed changes

src/tensor_ops/cmp/mod.rs Show resolved Hide resolved

src/tensor_ops/cmp/mod.rs Show resolved Hide resolved

ViliamVadocz added 4 commits February 3, 2023 16:54

Merge branch 'main' into comparison-ops

03a95f2

Assert tensor shapes are equal in comparison ops

3523825

Add test to check that shape mismatch is caught

dff48c5

Add TODO to remove cfg once Cuda supports integers

78b40f1

ViliamVadocz requested a review from coreylowman February 4, 2023 00:24

coreylowman reviewed Feb 6, 2023

View reviewed changes

src/tensor_ops/cmp/mod.rs

}

#[test]

#[should_panic]

Copy link

Owner

coreylowman Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test 😀

coreylowman approved these changes Feb 6, 2023

View reviewed changes

coreylowman merged commit 79d581f into coreylowman:main Feb 9, 2023

ViliamVadocz deleted the comparison-ops branch February 9, 2023 16:06

Uh oh!

Add comparison tensor operations #386

Add comparison tensor operations #386

Uh oh!

Conversation

ViliamVadocz commented Jan 21, 2023

Uh oh!

ViliamVadocz commented Jan 21, 2023

Uh oh!

nkoppel commented Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViliamVadocz commented Jan 21, 2023

Uh oh!

nkoppel commented Jan 22, 2023

Uh oh!

coreylowman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ViliamVadocz commented Jan 30, 2023

Uh oh!

nkoppel commented Jan 30, 2023

Uh oh!

coreylowman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ViliamVadocz commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coreylowman Feb 6, 2023

Choose a reason for hiding this comment

Uh oh!

coreylowman left a comment

Choose a reason for hiding this comment

Uh oh!

coreylowman commented Feb 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nkoppel commented Jan 21, 2023 •

edited

Loading

ViliamVadocz commented Feb 3, 2023 •

edited

Loading