Add Salt and Pepper noise CPU operator #2889

jantonguirao · 2021-04-21T16:58:57Z

Signed-off-by: Joaquin Anton janton@nvidia.com

Why we need this PR?

Pick one, remove the rest

It adds a new feature needed because to generate Salt and Pepper noise in DALI. A separate PR will follow with the GPU implementation.

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

What solution was applied:
Implemented salt and pepper CPU operator in terms of the existing random noise template operator
Affected modules and functionalities:
New operator
Key points relevant for the review:
RNGBase changes, SaltAndPepper implementation
Validation and testing:
Python Tests added
Documentation (including examples):
Docstr added

JIRA TASK: [DALI-1969]

dali/operators/random/noise/salt_and_pepper_noise.cc

JanuszL · 2021-04-28T20:39:58Z

dali/operators/random/noise/salt_and_pepper_noise.h

+        salt_prob_(noise_prob_ * clamp<float>(salt_to_pepper_prob, 0, 1)),
+        salt_val_(ConvertNorm<T>(1.0f)),
+        pepper_val_(std::is_unsigned<T>::value ? ConvertSatNorm<T>(0.0f) :
+                                                 ConvertSatNorm<T>(-1.0f)) {}


So the assumption is that float images has range from -1 to 1, what if someone has uint8 based image and then convert it to float?

JanuszL · 2021-04-28T20:51:24Z

dali/operators/random/noise/salt_and_pepper_noise.h

+
+  template <typename T>
+  bool SetupDists(typename Dist<T>::type* dists_data, int nsamples) {
+    if (!prob_.IsDefined() && !salt_to_pepper_prob_.IsDefined()) {


If no "prob" and "salt_to_pepper_prob" are provided you just get a uniform random noise instead of the image?
Or just default salt_and_pepper_noise_impl will be used?

In case of return false, the default constructed implementation is used.

What if the probabilities are left a default, but salt and pepper values are not?

True, that's a bug

dali/test/python/test_operator_noise_salt_and_pepper.py

JanuszL · 2021-04-28T21:14:23Z

dali/test/python/test_operator_noise_salt_and_pepper.py

+@pipeline_def
+def pipe_salt_and_pepper_noise(prob, salt_to_pepper_prob, device='cpu'):
+    encoded, _ = fn.readers.file(file_root=images_dir)
+    in_data = fn.decoders.image(encoded, device="cpu", output_type=types.RGB)


Suggested change

in_data = fn.decoders.image(encoded, device="cpu", output_type=types.RGB)

in_data = fn.decoders.image(encoded, output_type=types.RGB)

dali/test/python/test_operator_noise_salt_and_pepper.py

Signed-off-by: Joaquin Anton <janton@nvidia.com>

dali/operators/random/noise/salt_and_pepper_noise.cc

JanuszL · 2021-05-12T17:13:24Z

dali/operators/random/noise/salt_and_pepper_noise.h

+      : noise_prob_(clamp<float>(noise_prob, 0, 1)),
+        salt_prob_(noise_prob_ * clamp<float>(salt_to_pepper_prob, 0, 1)),


Do we want to clamp or enforce ranges in L112.

I'll change to enforce.

Is this constructor (with non-default arguments) ever used in device code? If not, we should probably throw std::range_error. If it is, clamping here is the right thing to do.
I suggest that you remove the default arguments from here and initialize the fields - hopefully with that you could mark this constructor as host-only and throw?

dali/operators/random/noise/salt_and_pepper_noise.h

JanuszL · 2021-05-12T22:27:31Z

dali/operators/random/noise/salt_and_pepper_noise.cc

+is generated once and applied to all channels, so that all channels in a pixel should either be
+kept intact, take the "pepper" value, or the "salt" value.
+
+Note: Per-channel noise generation requires the input layout to contain a channels ('C') dimension.)code",


L138 in dali/operators/random/rng_base_cpu.h states that the layout can be empty and channel-last is assumed

I'll update this note.

JanuszL · 2021-05-12T22:29:47Z

dali/operators/random/rng_base_cpu.h

  DistGen<IsNoiseGen> dist_gen_;
  for (int sample_id = 0; sample_id < nsamples; ++sample_id) {
    auto sample_sz = out_shape.tensor_size(sample_id);
+    int64_t N = sample_sz;


Can you rename N to something more meaningful?

JanuszL · 2021-05-12T22:57:33Z

dali/test/python/test_operator_noise_salt_and_pepper.py

+    passthrough_mask = np.all(output == input, axis=-1)
+    pepper_mask = np.all(output == pepper_val, axis=-1)
+    salt_mask = np.all(output == salt_val, axis=-1)
+    pixel_mask = np.logical_and(np.all(input != pepper_val, axis=-1),


How it is different from passthrough_mask?

Now I see. How about renaming it to input_pixel_mask?

I would add a comment that you want to filter out pixels in the input that already have salt/pepper values.

dali/test/python/test_operator_noise_salt_and_pepper.py

Signed-off-by: Joaquin Anton <janton@nvidia.com>

mzient · 2021-05-13T08:18:18Z

dali/operators/random/noise/salt_and_pepper_noise.cc

+    .AddOptionalArg<float>("prob",
+      R"code(Probability of an output value to take a salt or pepper value.)code",
+      0.05f, true)
+    .AddOptionalArg<float>("salt_to_pepper_prob",


Suggested change

.AddOptionalArg<float>("salt_to_pepper_prob",

.AddOptionalArg<float>("salt_vs_pepper",

The "salt_to_pepper" suggests a ratio and this is not a ratio but rather relative amount of salt.

dali/operators/random/noise/salt_and_pepper_noise.h

dali/operators/random/noise/salt_and_pepper_noise.cc

Signed-off-by: Joaquin Anton <janton@nvidia.com>

mzient · 2021-05-13T11:49:08Z

dali/operators/random/noise/salt_and_pepper_noise.h

+      assert(0.0f <= noise_prob && noise_prob <= 1.0);
+      assert(0.0f <= salt_vs_pepper && salt_vs_pepper <= 1.0);


Please remove this from device code. Asserts cost, sometimes a lot. Do you even know if this works in debug mode without exceeding launch resources?

I've tested it in debug mode. Anyway, removed the assert.

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao · 2021-05-13T11:59:49Z

!build

dali-automaton · 2021-05-13T12:06:35Z

CI MESSAGE: [2366259]: BUILD STARTED

dali-automaton · 2021-05-13T13:33:12Z

CI MESSAGE: [2366259]: BUILD PASSED

jantonguirao force-pushed the salt_and_pepper branch from 2a9f4b6 to fafec94 Compare April 27, 2021 16:27

jantonguirao changed the title ~~[WIP] Add Salt and Pepper noise CPU and GPU operator~~ Add Salt and Pepper noise CPU and GPU operator Apr 27, 2021

jantonguirao marked this pull request as ready for review April 27, 2021 16:29

JanuszL self-assigned this Apr 27, 2021

mzient reviewed Apr 27, 2021

View reviewed changes

dali/operators/random/noise/salt_and_pepper_noise.cc Show resolved Hide resolved

jantonguirao assigned mzient Apr 28, 2021

JanuszL reviewed Apr 28, 2021

View reviewed changes

dali/test/python/test_operator_noise_salt_and_pepper.py Outdated Show resolved Hide resolved

JanuszL reviewed Apr 28, 2021

View reviewed changes

dali/test/python/test_operator_noise_salt_and_pepper.py Show resolved Hide resolved

jantonguirao marked this pull request as draft May 10, 2021 13:39

jantonguirao force-pushed the salt_and_pepper branch from fafec94 to f952e02 Compare May 10, 2021 16:31

jantonguirao added 7 commits May 12, 2021 10:41

WIP

42ed235

Signed-off-by: Joaquin Anton <janton@nvidia.com>

Add tests

708e097

Signed-off-by: Joaquin Anton <janton@nvidia.com>

RNGBase: Separate noise generation and application steps

293a110

Signed-off-by: Joaquin Anton <janton@nvidia.com>

WIP per-channel=False generation

332ecd2

Signed-off-by: Joaquin Anton <janton@nvidia.com>

WIP

e254f8b

Signed-off-by: Joaquin Anton <janton@nvidia.com>

Fix rebase merge

d1204d3

Signed-off-by: Joaquin Anton <janton@nvidia.com>

Working CPU SaltAndPepper

1449eb1

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao force-pushed the salt_and_pepper branch from bea49b4 to 1449eb1 Compare May 12, 2021 14:57

Removing CPU implementation from this PR

6198faa

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao force-pushed the salt_and_pepper branch from c439ede to 6198faa Compare May 12, 2021 15:16

jantonguirao changed the title ~~Add Salt and Pepper noise CPU and GPU operator~~ Add Salt and Pepper noise CPU operator May 12, 2021

Custom salt and pepper values

53ebd4d

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao marked this pull request as ready for review May 12, 2021 16:26

JanuszL reviewed May 12, 2021

View reviewed changes

dali/operators/random/noise/salt_and_pepper_noise.cc Show resolved Hide resolved

JanuszL reviewed May 12, 2021

View reviewed changes

dali/operators/random/noise/salt_and_pepper_noise.h Show resolved Hide resolved

JanuszL reviewed May 12, 2021

View reviewed changes

dali/test/python/test_operator_noise_salt_and_pepper.py Outdated Show resolved Hide resolved

JanuszL reviewed May 12, 2021

View reviewed changes

dali/test/python/test_operator_noise_salt_and_pepper.py Show resolved Hide resolved

Code review fixes

6ed4421

Signed-off-by: Joaquin Anton <janton@nvidia.com>

mzient reviewed May 13, 2021

View reviewed changes

JanuszL reviewed May 13, 2021

View reviewed changes

dali/operators/random/noise/salt_and_pepper_noise.h Outdated Show resolved Hide resolved

JanuszL reviewed May 13, 2021

View reviewed changes

dali/operators/random/noise/salt_and_pepper_noise.cc Show resolved Hide resolved

JanuszL approved these changes May 13, 2021

View reviewed changes

Code review fixes

0113c20

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao force-pushed the salt_and_pepper branch from cd45ea8 to 0113c20 Compare May 13, 2021 11:40

mzient reviewed May 13, 2021

View reviewed changes

Remove assert from device code

90ef0a2

Signed-off-by: Joaquin Anton <janton@nvidia.com>

mzient approved these changes May 13, 2021

View reviewed changes

jantonguirao merged commit 7e8b890 into NVIDIA:master May 13, 2021

JanuszL mentioned this pull request May 20, 2021

DALI 2021 roadmap #2978

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Salt and Pepper noise CPU operator #2889

Add Salt and Pepper noise CPU operator #2889

jantonguirao commented Apr 21, 2021 •

edited

Loading

JanuszL Apr 28, 2021

JanuszL Apr 28, 2021 •

edited

Loading

jantonguirao May 11, 2021

mzient May 13, 2021

jantonguirao May 13, 2021

JanuszL Apr 28, 2021

jantonguirao May 12, 2021

JanuszL May 12, 2021 •

edited

Loading

jantonguirao May 13, 2021

mzient May 13, 2021 •

edited

Loading

JanuszL May 12, 2021

jantonguirao May 13, 2021

JanuszL May 12, 2021

jantonguirao May 13, 2021

JanuszL May 12, 2021

JanuszL May 12, 2021

JanuszL May 12, 2021

jantonguirao May 13, 2021

mzient May 13, 2021

mzient May 13, 2021

jantonguirao May 13, 2021

jantonguirao commented May 13, 2021

dali-automaton commented May 13, 2021

dali-automaton commented May 13, 2021

	in_data = fn.decoders.image(encoded, device="cpu", output_type=types.RGB)
	in_data = fn.decoders.image(encoded, output_type=types.RGB)

		: noise_prob_(clamp<float>(noise_prob, 0, 1)),
		salt_prob_(noise_prob_ * clamp<float>(salt_to_pepper_prob, 0, 1)),

	.AddOptionalArg<float>("salt_to_pepper_prob",
	.AddOptionalArg<float>("salt_vs_pepper",

		assert(0.0f <= noise_prob && noise_prob <= 1.0);
		assert(0.0f <= salt_vs_pepper && salt_vs_pepper <= 1.0);

Add Salt and Pepper noise CPU operator #2889

Add Salt and Pepper noise CPU operator #2889

Conversation

jantonguirao commented Apr 21, 2021 • edited Loading

Why we need this PR?

What happened in this PR?

Choose a reason for hiding this comment

JanuszL Apr 28, 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

JanuszL May 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient May 13, 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

jantonguirao commented May 13, 2021

dali-automaton commented May 13, 2021

dali-automaton commented May 13, 2021

jantonguirao commented Apr 21, 2021 •

edited

Loading

JanuszL Apr 28, 2021 •

edited

Loading

JanuszL May 12, 2021 •

edited

Loading

mzient May 13, 2021 •

edited

Loading