Add Laplacian CPU kernel #3518

stiepan · 2021-11-19T20:00:24Z

Signed-off-by: Kamil Tokarski [email protected]

Add Laplacian CPU kernel

Description

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactoring (Redesign of existing code that doesn't affect functionality)
Other (e.g. Documentation, Tests, Configuration)

What happened in this PR

PR adds laplacian cpu kernel along with a few gtest tests.
It boils down to running a few separable convolutions and summing the results - specializations purpose is to first allocate (or use output buffer if applicable) an intermediate buffer for accumulating the results and to pass appropriate transforms to the convolution so that the convolutions results are accumulated in the same pass as the convolution computation.

Additional information

Key points relevant for the review:

Checklist

Tests

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: DALI-2471

stiepan · 2021-11-19T20:01:36Z

!build

dali-automaton · 2021-11-19T20:06:11Z

CI MESSAGE: [3438316]: BUILD STARTED

dali-automaton · 2021-11-19T21:34:01Z

CI MESSAGE: [3438316]: BUILD FAILED

dali-automaton · 2021-11-22T21:12:35Z

CI MESSAGE: [3438316]: BUILD PASSED

stiepan · 2021-11-23T12:22:05Z

!build

dali-automaton · 2021-11-23T12:25:56Z

CI MESSAGE: [3459044]: BUILD STARTED

dali-automaton · 2021-11-23T17:05:07Z

CI MESSAGE: [3459044]: BUILD PASSED

stiepan · 2021-11-24T17:23:58Z

!build

dali-automaton · 2021-11-24T17:26:27Z

CI MESSAGE: [3467428]: BUILD STARTED

dali-automaton · 2021-11-24T18:36:35Z

CI MESSAGE: [3467428]: BUILD PASSED

klecki

I feel like the axis order is reversed in Laplacian compared to what we do in the Convolutions.

Some nitpicks.

Also, I think that the convolution changes should go as a separate PR (I could quickly approve the convolution + tests changes), and the laplacian + laplacian test (I didn't review the test yet) should go to a separate one.

klecki · 2021-11-24T15:30:01Z

dali/kernels/imgproc/convolution/convolution_cpu.h

+namespace conv_transform {
+
+/**
+ * @brief Transforms enable postprocessing of values computed by 1D convolution before


Just a nitpick, but this docstring is only for the TransScaleSat, you may add a group surrounding the classes below.

Changed that in a separate PR.
#3535

klecki · 2021-11-24T16:03:49Z

dali/kernels/imgproc/convolution/convolution_cpu.h

+    out_ptr[offset] = ConvertSat<Out>(val * scale);
+  }
+
+  float scale;


I assume, that the default constructor due to the default argument initializes it to 1.0f?
Maybe we should just slap a

Suggested change

float scale;

float scale = 1.f;

here?

However, it may be the case that somebody wants to specify different scaling factor, right? In that case this constructor is still needed.

klecki · 2021-11-25T11:28:25Z

dali/kernels/imgproc/convolution/laplacian_cpu.h

+ */
+template <typename Out, typename In, typename W, int axes, int deriv_axis,
+          bool has_channels = false, typename T = conv_transform::TransScaleSat<Out, W>>
+struct Convolution {


I think, as this is mainly used for the calculation of derivatives, we should name it a bit different than just Convolution.

Renamed it to PartialDeriv

klecki · 2021-11-25T14:28:45Z

dali/kernels/imgproc/convolution/laplacian_cpu.h

+struct Convolution {
+  using MultiDimConv = SeparableConvolutionCpu<Out, In, W, axes, has_channels, T>;
+  static constexpr int ndim = MultiDimConv::ndim;
+  using SingleDimConv = ConvolutionCpu<Out, In, W, ndim, deriv_axis, has_channels, T>;


In case of DxKernel we placed deriv_axis=0, but the convolution assumes that x in HW layout is the last one, so I would expect 1 to be used here.

I reversed the x, y, z naming of the sub kernels.

klecki · 2021-11-25T16:30:49Z

dali/kernels/imgproc/convolution/laplacian_cpu.h

+    sub_ctx.scratchpad = &sub_scratch;
+
+    // Clear the scratchpad for sub-kernels to reuse memory
+    sobel_dx_.Run(sub_ctx, acc, in, windows[0], scale[0]);


Suggested change

sobel_dx_.Run(sub_ctx, acc, in, windows[0], scale[0]);

sobel_dx_.Run(sub_ctx, acc, in, windows[0], {scale[0]});

Shouldn't we create a transform here? How does it work with a scale? Is it implicit conversion or something?

Also, we pass 2 scales (axes = 2) and only use the one here. Is it intended?

Same in the 3D case.

Oh, I see it's packed into the separate transform.

klecki · 2021-11-25T16:38:57Z

dali/kernels/imgproc/convolution/laplacian_cpu.h

+          bool has_channels>
+struct LaplacianCPUBase<T, Intermediate, Out, In, W, 2, has_channels> {
+  static constexpr int axes = 2;
+  using DxKernel = Convolution<Intermediate, In, W, axes, 0, has_channels,


As mentioned above, the convolution in 2D case, assumes the HW[C] layout, so the x is typically the second (index 1) data axis.

I reversed the x, y, z naming of the sub kernels.

klecki · 2021-11-25T16:40:38Z

dali/kernels/imgproc/convolution/laplacian_cpu.h

+  void Run(KernelContext& ctx, const TensorView<StorageCPU, Out, ndim> &out,
+           const TensorView<StorageCPU, Intermediate, ndim> &acc,
+           const TensorView<StorageCPU, const In, ndim>& in,
+           const std::array<std::array<TensorView<StorageCPU, const W, 1>, axes>, axes>& windows,


I'm thinking that we might need some docs about the nesting of windows at least :)

I've added a brief docstr to the LaplacianCPU declaration that describes the ordering used in windows-related arguments.

klecki · 2021-11-25T16:58:16Z

dali/kernels/imgproc/convolution/laplacian_cpu_test.cc

+    std::array<float, window_size> w = {0.};
+    w[0] = 1.;
+    for (int i = 1; i < window_size - d_order; i++) {
+    auto prevval = w[0];


Broken indentation.

Should be fine now.

klecki · 2021-11-25T17:05:17Z

dali/kernels/imgproc/convolution/laplacian_cpu_test.cc

+  LaplacianWindows() {
+    for (int i = 0; i < axes; i++) {
+      for (int j = 0; j < axes; j++) {
+        if (i == j) {
+          window_sizes[i][j] = window_size;
+          windows[i][j] = GetSobelWindow<window_size>(2);
+          tensor_windows[i][j] = {windows[i][j].data(), window_size};
+        } else if (use_smoothing) {
+          window_sizes[i][j] = window_size;
+          windows[i][j] = GetSobelWindow<window_size>(0);
+          tensor_windows[i][j] = {windows[i][j].data(), window_size};
+        } else {
+          window_sizes[i][j] = 1;
+          windows[i][j] = uniform_array<window_size>(0.f);
+          auto middle = window_size / 2;
+          windows[i][j][middle] = 1.f;
+          tensor_windows[i][j] = {windows[i][j].data() + middle, 1};
+        }
+      }
+    }
+  }
+  std::array<std::array<int, axes>, axes> window_sizes;
+  std::array<std::array<std::array<float, window_size>, axes>, axes> windows;
+  std::array<std::array<TensorView<StorageCPU, const float, 1>, axes>, axes> tensor_windows;


Again, I am lost about which level of nesting corresponds to what.

It goes the same way as in LaplacianCPU.Run. To recap:
tensor_windows[i] describes windows used to compute the i-th partial derivative (i.e. the one that approximates the second order partial derivative along i-th dimension when counting them from the left to the right). So tensor_windows[i][i] is a window that should look like alike [1, -2, 1], whereas for j <> i, tensor_windows[i][j] is some kind of a smoothing window.

Signed-off-by: Kamil Tokarski <[email protected]>

stiepan · 2021-11-28T20:14:39Z

!build

dali-automaton · 2021-11-28T20:21:10Z

CI MESSAGE: [3487879]: BUILD STARTED

dali-automaton · 2021-11-29T03:21:18Z

CI MESSAGE: [3487879]: BUILD FAILED

dali-automaton · 2021-11-29T07:30:51Z

CI MESSAGE: [3487879]: BUILD PASSED

dali/kernels/imgproc/convolution/laplacian_cpu.h

szalpal · 2021-12-06T09:10:57Z

dali/kernels/imgproc/convolution/laplacian_cpu.h

+ * window of size 1 must be equal to `[1]`, this way, if window sizes in non-derivative directions
+ * are one, the smoothing convolutions can be skipped and only a single one-dimensional


Suggested change

* window of size 1 must be equal to `[1]`, this way, if window sizes in non-derivative directions

* are one, the smoothing convolutions can be skipped and only a single one-dimensional

* window of size 1 must be equal to `[1]`. This way, if window sizes in non-derivative directions

* are one, the smoothing convolutions can be skipped and only a single one-dimensional

And I feel there's something wrong with the latter sentence:

This way, if window sizes in non-derivative directions are one [...]

Should it be is one? Or maybe is [1]?

For 3D case there are in fact two smoothing window sizes for each partial derivative. This optimization handles the case where all the smoothing window sizes are 1, hence "are".

dali/kernels/imgproc/convolution/laplacian_cpu.h

szalpal · 2021-12-06T09:27:52Z

dali/kernels/imgproc/convolution/laplacian_cpu.h

+
+namespace laplacian {
+
+using namespace conv_transform;  // NOLINT


Do we need this here? I think I saw only a few references to this namespaces, maybe it would be cleaner to do it explicitly?

dali/kernels/imgproc/convolution/laplacian_cpu.h

Signed-off-by: Kamil Tokarski <[email protected]>

stiepan · 2021-12-07T11:02:18Z

!build

dali-automaton · 2021-12-07T11:06:12Z

CI MESSAGE: [3544355]: BUILD STARTED

dali-automaton · 2021-12-07T12:16:18Z

CI MESSAGE: [3544355]: BUILD PASSED

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

stiepan force-pushed the laplacian_cpu_kernel branch from dea2303 to 7a05d36 Compare November 23, 2021 10:53

stiepan marked this pull request as ready for review November 23, 2021 10:55

jantonguirao assigned banasraf and klecki Nov 23, 2021

stiepan force-pushed the laplacian_cpu_kernel branch from 71a0bd6 to 354c550 Compare November 24, 2021 11:19

klecki reviewed Nov 25, 2021

View reviewed changes

stiepan added 2 commits November 28, 2021 12:04

Add laplacian CPU kernel

b8edddb

Signed-off-by: Kamil Tokarski <[email protected]>

Rename axes

5e2cf9a

Signed-off-by: Kamil Tokarski <[email protected]>

stiepan force-pushed the laplacian_cpu_kernel branch from 354c550 to 5e2cf9a Compare November 28, 2021 17:25

klecki approved these changes Nov 29, 2021

View reviewed changes

stiepan unassigned banasraf Nov 30, 2021

jantonguirao assigned szalpal Nov 30, 2021

szalpal reviewed Dec 6, 2021

View reviewed changes

Fix scratchpad append

a187516

Signed-off-by: Kamil Tokarski <[email protected]>

stiepan added 3 commits December 7, 2021 10:39

Review remarks

e5a9015

Signed-off-by: Kamil Tokarski <[email protected]>

Refer to SeparableConvolutionCpu in docs

c0b4418

Signed-off-by: Kamil Tokarski <[email protected]>

Rename LaplacianCPU

97e43d6

Signed-off-by: Kamil Tokarski <[email protected]>

szalpal approved these changes Dec 7, 2021

View reviewed changes

stiepan merged commit 7ea3dfb into NVIDIA:main Dec 8, 2021

cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

07f5a51

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

46afb16

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

a54075a

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

d57afce

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

14a7d6d

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

2510ac2

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

4bffc8e

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

1b484ee

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

871a086

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

d39cb57

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

2a5b848

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

ef6a807

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

65f07d6

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

cyyever pushed a commit to cyyever/DALI that referenced this pull request Feb 21, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

3f8b9d2

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

JanuszL mentioned this pull request Mar 30, 2022

DALI 2022 roadmap #3774

Closed

cyyever pushed a commit to cyyever/DALI that referenced this pull request May 13, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

7d3f3fa

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

cyyever pushed a commit to cyyever/DALI that referenced this pull request Jun 7, 2022

Add Laplacian CPU kernel (NVIDIA#3518)

ed69f3d

* Add laplacian CPU kernel Signed-off-by: Kamil Tokarski <[email protected]>

	sobel_dx_.Run(sub_ctx, acc, in, windows[0], scale[0]);
	sobel_dx_.Run(sub_ctx, acc, in, windows[0], {scale[0]});

		* window of size 1 must be equal to `[1]`, this way, if window sizes in non-derivative directions
		* are one, the smoothing convolutions can be skipped and only a single one-dimensional


		namespace laplacian {

		using namespace conv_transform; // NOLINT

Add Laplacian CPU kernel #3518

Add Laplacian CPU kernel #3518

Conversation

stiepan commented Nov 19, 2021 • edited Loading

Description

What happened in this PR

Additional information

Checklist

Tests

Documentation

DALI team only

Requirements

stiepan commented Nov 19, 2021

dali-automaton commented Nov 19, 2021

dali-automaton commented Nov 19, 2021

dali-automaton commented Nov 22, 2021

stiepan commented Nov 23, 2021

dali-automaton commented Nov 23, 2021

dali-automaton commented Nov 23, 2021

stiepan commented Nov 24, 2021

dali-automaton commented Nov 24, 2021

dali-automaton commented Nov 24, 2021

klecki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stiepan Nov 25, 2021 • 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

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

stiepan commented Nov 28, 2021

dali-automaton commented Nov 28, 2021

dali-automaton commented Nov 29, 2021

dali-automaton commented Nov 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stiepan commented Dec 7, 2021

dali-automaton commented Dec 7, 2021

dali-automaton commented Dec 7, 2021

stiepan commented Nov 19, 2021 •

edited

Loading

stiepan Nov 25, 2021 •

edited

Loading