Reduce Sum Op #2379

awolant · 2020-10-19T18:55:30Z

Why we need this PR?

It adds new feature needed because of we want to support all reductions

What happened in this PR?

What solution was applied:
Added SumOp using CRTP to reuse existing reductions code, type mapping implemented with new macro TYPE_MAP
Affected modules and functionalities:
Operators (new op), include (new macro)
Key points relevant for the review:
TYPE_MAP macro, Sum Op implementation
Validation and testing:
Added python tests for Sum Op
Documentation (including examples):
Added docs for new op, added comments for TYPE_MAP macro

JIRA TASK: [Use DALI-1675]

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant · 2020-10-19T19:45:04Z

!build

dali-automaton · 2020-10-19T20:29:36Z

CI MESSAGE: [1714406]: BUILD FAILED

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant · 2020-10-20T12:47:16Z

!build

dali-automaton · 2020-10-20T12:50:32Z

CI MESSAGE: [1716680]: BUILD STARTED

dali-automaton · 2020-10-20T17:23:33Z

CI MESSAGE: [1716680]: BUILD PASSED

JanuszL · 2020-10-21T09:10:44Z

dali/operators/generic/reduce/sum.h

+#include "include/dali/core/static_map.h"
+#include "dali/operators/generic/reduce/reduce.h"
+
+#define SUM_TYPES_MAP ( \


Please add a comment what is the format and what this map is for.

Done. Added comment here and more elaborate doc style comments for TYPE_MAP with its implementation.

JanuszL · 2020-10-21T09:11:47Z

dali/test/python/test_operator_reduce.py

@@ -5,6 +5,18 @@
 from nvidia.dali.pipeline import Pipeline
 import numpy as np

+to_dali_type = {


Please use np_types_to_dali from test_utils.py

JanuszL · 2020-10-21T09:13:44Z

dali/test/python/test_operator_reduce.py

+        (np.uint32, [np.uint64, np.float32]),
+        (np.int32, [np.int32, np.int64, np.float32])]
+
+    for keep_dims in [False, True]:


How long does this test take. Maybe we should split it to smaller and bigger flavor?

This whole file as it is in this PR takes around 30 seconds to run, so I don't think it's a problem.

jantonguirao · 2020-10-22T10:56:59Z

dali/operators/generic/reduce/reduce.cc

@@ -41,15 +51,21 @@ DALI_SCHEMA(Max)
  .NumOutput(1)
  .AddParent("ReduceBase");

-using MinCPU = Reduce<kernels::MinCPU, CPUBackend>;
+using SumCPU = SumOp<kernels::SumCPU, CPUBackend>;
+DALI_REGISTER_OPERATOR(Sum, SumCPU, CPU);


Didn't we want to have all the reductions in a reductions namespace?
If we haven't released yet those it's as simple as renaming "Sum" to "reductions__Sum" and so on.

We have that in some nightly, but I can change that no problem. Let's discuss.

Done, moved to reductions

jantonguirao · 2020-10-22T10:58:05Z

Acknowledgements.txt

+
+========================
+
+uSHET Library - CPP Magic


Maybe you could mention why we are ack-ing this for the future reader.

Added comment to relevant file - static_map.h

jantonguirao · 2020-10-22T10:58:28Z

dali/operators/generic/reduce/reduce.cc

@@ -29,6 +30,15 @@ Not providing any axis results in reduction of all elements.)code",
    "If True, maintains original input dimensions.",
    false);

+DALI_SCHEMA(Sum)
+  .DocStr("")


missing documentation

jantonguirao · 2020-10-22T10:59:52Z

dali/operators/generic/reduce/reduce.h

 class Reduce : public Operator<Backend> {
 public:
  explicit inline Reduce(const OpSpec &spec) :
    Operator<Backend>(spec),
    axes_(spec.GetRepeatedArgument<int>("axes")),
    keep_dims_(spec.GetArgument<bool>("keep_dims")) {
+      if (!spec.TryGetArgument<DALIDataType>(output_type_, "dtype")) {


Suggested change

if (!spec.TryGetArgument<DALIDataType>(output_type_, "dtype")) {

output_type_ = spec.GetArgument<DALIDataType>("dtype");

you already have a default value in the schema

This is some leftover from previous version.

jantonguirao · 2020-10-22T11:00:23Z

dali/operators/generic/reduce/reduce.h

-    TYPE_SWITCH(data_type, type2id, DataType, REDUCE_TYPES, (
-      RunTyped<DataType, DataType>(ws);),
-      DALI_FAIL(make_string("Unsupported input type: ", data_type)))
+    ImplType<ReductionType, Backend>& reduce_impl =


Suggested change

ImplType<ReductionType, Backend>& reduce_impl =

auto& reduce_impl =

?

jantonguirao · 2020-10-22T11:03:13Z

dali/operators/generic/reduce/reduce.h

+    DALIDataType input_type = in.type().id();
+
+    TYPE_SWITCH(input_type, type2id, DataType, REDUCE_TYPES, (
+      Reduce<ReductionType, Backend, ReduceOp>& base =


Suggested change

Reduce<ReductionType, Backend, ReduceOp>& base =

auto& base =

jantonguirao · 2020-10-22T11:07:21Z

dali/operators/generic/reduce/reduce.h

+ public:
+  explicit inline ReduceOp(const OpSpec &spec) :  Reduce<ReductionType, Backend, ReduceOp>(spec) {}
+
+  void RunImplImpl(workspace_t<Backend> &ws) {


RunImplImpl? :) I think that's too much. Why not overriding RunImpl directly?

Because this is not inheritance but CRTP. I can change the name but it is quite accurate. RunImpl calls implementations of RunImplImpl based on last template parameter.
This way I can share most of the code for reduction and only change RunImplImpl based on ReductionType template parameter.
Maybe it's not super clear now, since Max and Min are using default implementation of RunImplImpl, but Sum changes that.

fair enough

jantonguirao · 2020-10-22T12:18:02Z

dali/operators/generic/reduce/sum.h

+    auto& in = ws.template InputRef<Backend>(0);
+    DALIDataType input_type = in.type().id();
+
+    Reduce<ReductionType, Backend, SumOp>& base =


Suggested change

Reduce<ReductionType, Backend, SumOp>& base =

auto& base =

Now done, I hope :)

jantonguirao · 2020-10-22T12:24:25Z

dali/test/python/test_operator_reduce.py

+        (np.uint32, [np.uint64, np.float32]),
+        (np.int32, [np.int32, np.int64, np.float32])]
+
+    for keep_dims in [False, True]:


maybe keep_dims could be a random choice in the inner part of the nested loops? You'll cut the number of tests in half and still cover what you need to test

This whole file as it is in this PR takes around 30 seconds to run, so I don't think it's a problem.

JanuszL · 2020-10-22T12:48:05Z

dali/test/python/test_operator_reduce.py

@@ -5,6 +5,18 @@
 from nvidia.dali.pipeline import Pipeline
 import numpy as np

+to_dali_type = {
+    np.int8: types.INT8,
+    np.uint8: types.UINT8,


Can you add a test to cpu only file?

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant · 2020-10-22T20:01:35Z

!build

dali-automaton · 2020-10-22T20:05:38Z

CI MESSAGE: [1725506]: BUILD STARTED

dali-automaton · 2020-10-22T22:56:55Z

CI MESSAGE: [1725506]: BUILD FAILED

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant · 2020-10-23T07:28:06Z

dali/test/python/test_dali_cpu_only.py

@@ -240,7 +240,7 @@ def test_mfcc_cpu():
    spectrum = fn.spectrogram(data, nfft = 60, window_length = 50, window_step = 25)
    mel = fn.mel_filter_bank(spectrum)
    dec = fn.to_decibels(mel)
-    processed = fn.mfc(dec)
+    processed = fn.mfcc(dec)


Not related typo fix.

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant · 2020-10-23T07:40:04Z

!build

jantonguirao · 2020-10-23T07:44:03Z

dali/operators/generic/reduce/sum.h

+    auto& in = ws.template InputRef<Backend>(0);
+    DALIDataType input_type = in.type().id();
+
+    Reduce<ReductionType, Backend, SumOp>& base =


dali-automaton · 2020-10-23T07:45:37Z

CI MESSAGE: [1727221]: BUILD STARTED

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant · 2020-10-23T08:41:39Z

!build

dali-automaton · 2020-10-23T08:45:27Z

CI MESSAGE: [1727370]: BUILD STARTED

dali-automaton · 2020-10-23T10:05:50Z

CI MESSAGE: [1727370]: BUILD PASSED

mzient · 2020-10-23T11:16:20Z

dali/operators/generic/reduce/sum.h

+    auto& base =
+      static_cast<Reduce<ReductionType, Backend, SumOp>&>(*this);
+    DALIDataType output_type = base.OutputType();


Suggested change

auto& base =

static_cast<Reduce<ReductionType, Backend, SumOp>&>(*this);

DALIDataType output_type = base.OutputType();

DALIDataType output_type = this->OutputType();

This should do the trick.

awolant

Tried that before, doesn't work.

mzient · 2020-10-23T11:21:31Z

dali/operators/generic/reduce/sum.h

+      static_cast<Reduce<ReductionType, Backend, SumOp>&>(*this);
+    DALIDataType output_type = base.OutputType();
+    if (output_type == DALI_NO_TYPE) {
+      output_type = input_type;


I still have my doubts here. This is going to be a major headache for the users of NumPy and PyTorch. The accumulator type for any integer is int64_t anyway, so it's not like we're saving anything by returning the result in a smaller type.

mzient · 2020-10-23T11:24:34Z

dali/operators/generic/reduce/reduce.h

@@ -144,6 +138,33 @@ class Reduce : public Operator<Backend> {
      false);
    kmgr_.Run<Kernel>(0, 0, ctx, out_view, in_view);
  }
+
+  DALIDataType OutputType() { return output_type_; }


Suggested change

DALIDataType OutputType() { return output_type_; }

DALIDataType OutputType() const { return output_type_; }

Signed-off-by: Albert Wolant <awolant@nvidia.com>

mzient · 2020-10-23T13:25:36Z

dali/operators/generic/reduce/sum.h

+    switch (input_type)
+    {


lint will complain

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant · 2020-10-23T13:38:09Z

!build

dali-automaton · 2020-10-23T13:45:35Z

CI MESSAGE: [1728169]: BUILD STARTED

dali-automaton · 2020-10-23T17:34:39Z

CI MESSAGE: [1728169]: BUILD PASSED

awolant added 2 commits October 13, 2020 21:21

Add sum op

6c20f54

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Sum op implementation

e47e965

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant changed the title ~~Reduce sum op~~ Reduce Sum Op Oct 19, 2020

awolant force-pushed the reduce_sum_op branch from 1a9c461 to e47e965 Compare October 19, 2020 19:44

awolant added 5 commits October 20, 2020 10:44

Merge branch 'master' into reduce_sum_op

e55bd4a

Fix license

03126f5

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Remove Mean

cbd5a8a

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Rename dtype param

ff2bfc0

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Merge branch 'master' into reduce_sum_op

b176284

JanuszL reviewed Oct 21, 2020

View reviewed changes

jantonguirao reviewed Oct 22, 2020

View reviewed changes

JanuszL reviewed Oct 22, 2020

View reviewed changes

awolant added 3 commits October 22, 2020 18:49

Static map tests

4da42e6

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Static map docs

10df331

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Fix lint

abba831

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Add CPU only tests

809ce9c

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant commented Oct 23, 2020

View reviewed changes

Move ops to reductions module

596a759

Signed-off-by: Albert Wolant <awolant@nvidia.com>

jantonguirao approved these changes Oct 23, 2020

View reviewed changes

Fix review

5b5ef71

Signed-off-by: Albert Wolant <awolant@nvidia.com>

JanuszL approved these changes Oct 23, 2020

View reviewed changes

mzient reviewed Oct 23, 2020

View reviewed changes

awolant commented Oct 23, 2020

View reviewed changes

mzient reviewed Oct 23, 2020

View reviewed changes

awolant added 3 commits October 23, 2020 13:28

Fix for review

56f3174

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Fix review

269cbb2

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Add output type promotion

c3d7c48

Signed-off-by: Albert Wolant <awolant@nvidia.com>

mzient reviewed Oct 23, 2020

View reviewed changes

Fix lint

195b731

Signed-off-by: Albert Wolant <awolant@nvidia.com>

mzient approved these changes Oct 23, 2020

View reviewed changes

awolant merged commit 35c91b6 into NVIDIA:master Oct 23, 2020

	if (!spec.TryGetArgument<DALIDataType>(output_type_, "dtype")) {
	output_type_ = spec.GetArgument<DALIDataType>("dtype");

	ImplType<ReductionType, Backend>& reduce_impl =
	auto& reduce_impl =

	Reduce<ReductionType, Backend, ReduceOp>& base =
	auto& base =

	DALIDataType OutputType() { return output_type_; }
	DALIDataType OutputType() const { return output_type_; }

Reduce Sum Op #2379

Reduce Sum Op #2379

Conversation

awolant commented Oct 19, 2020 • edited Loading

Why we need this PR?

What happened in this PR?

awolant commented Oct 19, 2020

dali-automaton commented Oct 19, 2020

awolant commented Oct 20, 2020

dali-automaton commented Oct 20, 2020

dali-automaton commented Oct 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awolant Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awolant commented Oct 22, 2020

dali-automaton commented Oct 22, 2020

dali-automaton commented Oct 22, 2020

Choose a reason for hiding this comment

awolant commented Oct 23, 2020

Choose a reason for hiding this comment

dali-automaton commented Oct 23, 2020

awolant commented Oct 23, 2020

dali-automaton commented Oct 23, 2020

dali-automaton commented Oct 23, 2020

mzient Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awolant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awolant commented Oct 23, 2020

dali-automaton commented Oct 23, 2020

dali-automaton commented Oct 23, 2020

awolant commented Oct 19, 2020 •

edited

Loading

awolant Oct 22, 2020 •

edited

Loading

mzient Oct 23, 2020 •

edited

Loading