Rework ops.random.CoinFlip #2577

jantonguirao · 2020-12-28T15:49:47Z

(#2531 should be merged first)

Why we need this PR?

Pick one, remove the rest

Refactoring to unify functionality of random number generators

What happened in this PR?

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

What solution was applied:
Implemented ops.random.CoinFlip in terms of RNGBase
Moved ops.CoinFlip to ops.random.CoinFlip
Affected modules and functionalities:
ops.random.CoinFlip
Key points relevant for the review:
CoinFlip implementation
Validation and testing:
Tests extended
Documentation (including examples):
Existing documentation

JIRA TASK: [DALI-1197]

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

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

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

review-notebook-app · 2020-12-28T15:49:52Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

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

banasraf

LGTM

dali/operators/random/coin_flip_cpu.cc

szalpal · 2021-01-18T14:23:35Z

dali/operators/util/randomizer.cuh

+    : probability_(probability) {}
+
+  __device__ inline bool operator()(curandState *state) const {
+    return curand_uniform(state) <= probability_ ? true : false;


How about

Suggested change

return curand_uniform(state) <= probability_ ? true : false;

return !(curand_uniform(state) > probability_);

;) Up to you

I could do return curand_uniform(state) <= probability_.
The negation just complicates things I believe

Yeah, that's true ;)

szalpal · 2021-01-18T14:32:04Z

dali/operators/random/coin_flip.h

+  DALIDataType DefaultDataType() const {
+    return DALI_INT32;
+  }


Is it used anywhere?

See my comment above

szalpal · 2021-01-18T14:36:10Z

dali/test/python/test_operator_coin_flip.py

+
+def test_coin_flip():
+    batch_size = 8
+    shape = [100000]


AFAIU, there's also a single-input version of this op, right? Maybe we should test it also?

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

szalpal · 2021-01-19T10:26:11Z

dali/test/python/test_operator_coin_flip.py

+def test_coin_flip():
+    batch_size = 8
+    for device in ['cpu', 'gpu']:
+        for max_shape, use_shape_like_in in [([100000], False),
+                                             ([100000], True),
+                                             (None, False)]:
+            for probability in [None, 0.7, 0.5, 0.0, 1.0]:
+                yield check_coin_flip, device, batch_size, max_shape, probability, use_shape_like_in


Probably it would be cleaner, if you'd go with two check... functions instead of the use_shape_like_in:

def test_coin_flip(): batch_size = 8 for device in ['cpu', 'gpu']: for max_shape in [100000, None]: for probability in [None, 0.7, 0.5, 0.0, 1.0]: yield check_coin_flip_input, device, batch_size, max_shape, probability yield check_coin_flip, device, batch_size, max_shape, probability

But that's up to you

I'd have to duplicate a lot of boilerplate, so I prefer it as is

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

jantonguirao · 2021-01-19T12:05:26Z

!build

dali-automaton · 2021-01-19T12:12:49Z

CI MESSAGE: [1993407]: BUILD STARTED

dali-automaton · 2021-01-19T13:54:10Z

CI MESSAGE: [1993407]: BUILD FAILED

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

jantonguirao · 2021-01-20T10:23:53Z

!build

dali-automaton · 2021-01-20T10:26:23Z

CI MESSAGE: [1997031]: BUILD STARTED

dali-automaton · 2021-01-20T15:12:19Z

CI MESSAGE: [1997031]: BUILD FAILED

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

jantonguirao · 2021-01-20T18:35:17Z

!build

dali-automaton · 2021-01-20T21:33:09Z

CI MESSAGE: [1998822]: BUILD STARTED

dali-automaton · 2021-01-21T01:49:23Z

CI MESSAGE: [1998822]: BUILD PASSED

jantonguirao and others added 25 commits December 7, 2020 11:15

WIP Rework random number generator ops

750f3df

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

Rework ArgValue utility

2ec4dc6

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

Code review fixes

bf22fad

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

Chunk RNG generation

8a03b7d

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

Code review fixes

67dfc51

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

Rework randomizer for better reuse

e1c2f0f

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

[WIP] GPU implementation

8aa2176

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

Remove old files

859a9cd

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

ArgValue utility

584bc15

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

Bug fixes and tests

2b98c1c

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

Merge branch 'argvalue_util' into rng_cpu_rework

70cca3e

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

Fixes after rebase

086afa5

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

Adjust for next operators needs

06ab984

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

Rework UniformDistribution CPU

c2d7714

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

Fix for default argument

bac3479

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

WIP

a90756e

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

Fix broken uniform operator python tests

9f52944

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

Review fixes

695dc28

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

Merge remote-tracking branch 'upstream/master' into uniform_rng_rework

397e259

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

WIP Uniform impl

63966f9

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

Bug fixes and tests in progress

04cac77

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

Merge remote-tracking branch 'upstream/master' into uniform_rng_rework

e9312d5

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

Replace ops.Uniform with ops.random.Uniform

7e3b443

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

Add CoinFlip rework

a965181

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

Replace deprecated usage

c3c756d

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

jantonguirao assigned szalpal and banasraf Jan 13, 2021

Merge remote-tracking branch 'upstream/master' into coin_flip_rng_rework

8e504d3

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

banasraf approved these changes Jan 14, 2021

View reviewed changes

szalpal reviewed Jan 18, 2021

View reviewed changes

jantonguirao added 2 commits January 18, 2021 17:16

Code review fixes

5247e37

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

Code review fixes

6b7d92c

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

szalpal approved these changes Jan 19, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into coin_flip_rng_rework

4474b13

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

Fix tests

dd2b2f9

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

jantonguirao added 2 commits January 20, 2021 19:30

Merge remote-tracking branch 'upstream/master' into coin_flip_rng_rework

26dea1e

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

Fix merge

e495670

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

jantonguirao merged commit f25cb53 into NVIDIA:master Jan 21, 2021

JanuszL mentioned this pull request Oct 26, 2021

DALI + Catalyst = 🚀 #3450

Closed

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework ops.random.CoinFlip #2577

Rework ops.random.CoinFlip #2577

jantonguirao commented Dec 28, 2020

review-notebook-app bot commented Dec 28, 2020

banasraf left a comment

szalpal Jan 18, 2021

jantonguirao Jan 18, 2021

szalpal Jan 18, 2021

szalpal Jan 18, 2021

jantonguirao Jan 18, 2021

szalpal Jan 18, 2021

jantonguirao Jan 18, 2021

szalpal Jan 19, 2021

jantonguirao Jan 20, 2021

jantonguirao commented Jan 19, 2021

dali-automaton commented Jan 19, 2021

dali-automaton commented Jan 19, 2021

jantonguirao commented Jan 20, 2021

dali-automaton commented Jan 20, 2021

dali-automaton commented Jan 20, 2021

jantonguirao commented Jan 20, 2021

dali-automaton commented Jan 20, 2021

dali-automaton commented Jan 21, 2021

	return curand_uniform(state) <= probability_ ? true : false;
	return !(curand_uniform(state) > probability_);

Rework ops.random.CoinFlip #2577

Rework ops.random.CoinFlip #2577

Conversation

jantonguirao commented Dec 28, 2020

Why we need this PR?

What happened in this PR?

review-notebook-app bot commented Dec 28, 2020

banasraf 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jantonguirao commented Jan 19, 2021

dali-automaton commented Jan 19, 2021

dali-automaton commented Jan 19, 2021

jantonguirao commented Jan 20, 2021

dali-automaton commented Jan 20, 2021

dali-automaton commented Jan 20, 2021

jantonguirao commented Jan 20, 2021

dali-automaton commented Jan 20, 2021

dali-automaton commented Jan 21, 2021