refact with Generator #905

fsx950223 · 2020-01-17T06:30:52Z

1.Remove "--benchmarks=all" will skip the benchmark test. Previous behavior will skip all test cases.
2.Refact rrelu with Generator which is introduced in Random number generation
3.Add graph test support

googlebot · 2020-02-26T14:31:04Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

gabrieldemarmiesse · 2020-02-26T14:48:41Z

@googlebot I consent

googlebot · 2020-02-26T14:48:49Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

gabrieldemarmiesse · 2020-02-26T14:59:11Z

I've merged master into your branch to update it and fixed any formatting/conflicts it might have. If you need to do some more modifications, please do git pull beforehand.

gabrieldemarmiesse

Thank you for the pull request, I'm in favor of such a change, being able to use random number generator is nice, but we may have to wait until this is not experimental anymore.

gabrieldemarmiesse · 2020-03-06T14:46:00Z

tensorflow_addons/activations/rrelu.py

@@ -26,6 +31,7 @@ def rrelu(
    upper: Number = 0.3333333333333333,
    training: Optional[bool] = None,
    seed: Optional[int] = None,
+    gs: Optional[tf.random.experimental.Generator] = None,


Do we have a policy about the use of experimental features in TensorFlow? What are the conditions in which we should accept the usage?

For example, I'd be ok if we use experimental features in tests, but not really in user-facing code. That's because it's ok if the build breaks because of an experimental feature, but it's not ok if the code the users are using is breaking.

@fsx950223 Maybe we can wait until the Generator is not experimental anymore?

Yes, I will wait until stable API is released

https://www.tensorflow.org/versions/r2.2/api_docs/python/tf/random/Generator

Stable API is available on TF2.2 which is what master builds against. Please merge master into this PR and should be good to update

gabrieldemarmiesse

Thank you for the pull request and thank you for waiting :)

gabrieldemarmiesse · 2020-05-02T10:19:44Z

tensorflow_addons/activations/rrelu.py

+    training: bool = None,
+    seed: Number = None,
+    gs: tf.random.Generator = None,


Please use Optional here. Having None as the default value doesn't make the type optional automatically. I think it should but it doesn't :(

gabrieldemarmiesse · 2020-05-02T10:21:25Z

tensorflow_addons/activations/rrelu.py

+from tensorflow_addons.utils.types import TensorLike, Number
+
+try:
+    tf.no_gradient("StatefulUniform")


I believe this deserves a comment as it's not obvious at all what is the purpose of this line.

Yes, I'm curiously the op doesn't register the gradient.
Any better solution?

I'm not sure I understand. You mean the tests will fail if this line is not used?

I test it with latest tf version and it seems the op has registered the gradient and I remove the tf.no_gradient.

gabrieldemarmiesse

Thanks for the changes, we're nearly there. We need to make sure we don't surprise the user.

gabrieldemarmiesse · 2020-05-02T11:25:34Z

tensorflow_addons/activations/rrelu.py

    lower: Number = 0.125,
    upper: Number = 0.3333333333333333,
    training: Optional[bool] = None,
    seed: Optional[int] = None,
+    gs: tf.random.Generator = None,


Suggested change

gs: tf.random.Generator = None,

gs: Optional[tf.random.Generator] = None,

gabrieldemarmiesse · 2020-05-02T11:36:26Z

tensorflow_addons/activations/rrelu.py

+    lower = tf.cast(lower, x.dtype)
+    upper = tf.cast(upper, x.dtype)
+    if gs is None:
+        gs = tf.random.get_global_generator()


Here we affect the global context, unlike before were the random global context wasn't affected. This is a breaking change potentially, especially if it's used with reset_from_seed. Also from the documentation :

This function will create the global generator the first time it is called, and the generator will be placed at the default device at that time, so one needs to be careful when this function is first called. Using a generator placed on a less-ideal device will incur performance regression.

So I believe that the behavior the rrelu should have when gs is None is the old one. We call tf.random.uniform and set the seed.

If the user provides his/her generator, then we use that (gs.uniform(...))

If the user provides a seed and a generator, we throw a ValueError because it makes no sense, the user is likely mistaken, with a nice error message of course.

If the user provides a seed and a generator, we throw a ValueError because it makes no sense, the user is likely mistaken, with a nice error message of course.

I remove gs or seed. But it casues unstable result when I test the code.

fsx950223 · 2020-05-07T02:19:57Z

cc @gabrieldemarmiesse

gabrieldemarmiesse

Thanks a lot for the changes, it's really nice. Just a few minor changes and we can merge.

gabrieldemarmiesse · 2020-05-07T15:25:37Z

tensorflow_addons/activations/rrelu.py

@@ -44,19 +44,28 @@ def rrelu(
        training: `bool`, indicating whether the `call`
        is meant for training or inference.
        seed: `int`, this sets the operation-level seed.
+        gs: A `Generator`. Default value is tf.random.get_global_generator().


We should change this line, it's not accurate anymore.

Also we should change the argument name to random_number_generator or rng whatever you prefer. It's more common to use this name.

gabrieldemarmiesse · 2020-05-07T15:28:14Z

tensorflow_addons/activations/rrelu.py

+                raise ValueError("Either seed or gs should be specific")
+            alpha = gs.uniform(tf.shape(x), minval=lower, maxval=upper, dtype=x.dtype)


Suggested change

raise ValueError("Either seed or gs should be specific")

alpha = gs.uniform(tf.shape(x), minval=lower, maxval=upper, dtype=x.dtype)

raise ValueError(

"Either seed or gs should be specified. Not both at the same time."

)

alpha = gs.uniform(tf.shape(x), minval=lower, maxval=upper, dtype=x.dtype)

gabrieldemarmiesse · 2020-05-07T15:30:56Z

tensorflow_addons/activations/rrelu.py

+        else:
+            if seed is not None:


In term of control flow, I believe something flatter would be easier to read. Something like:

if gs is not None and seed is not None: raise ValueError(...) elif gs is not None: gs.uniform(...) else: tf.random.uniform(..., seed=seed)

gabrieldemarmiesse · 2020-05-07T15:35:55Z

tensorflow_addons/activations/tests/rrelu_test.py

@@ -23,43 +23,37 @@
 SEED = 111111


+def rrelu_wrapper(lower, upper, training):


This function seems unused.

gabrieldemarmiesse · 2020-05-07T15:45:58Z

tensorflow_addons/activations/tests/rrelu_test.py

+    training_results = {
+        np.float16: [-0.3826, -0.165, 0, 1, 2],
+        np.float32: [-0.282151192, -0.199812651, 0, 1, 2],
+        np.float64: [-0.25720977, -0.1221586, 0, 1, 2],
+    }
+    gs = tf.random.Generator.from_seed(SEED)
+    result = rrelu(x, lower, upper, training=training, seed=None, gs=gs)


We shouldn't modify an existing test when testing a new option. It should be a new standalone test.

In the new test, rather than hardcoding the results, we could check that the generator works as expected, meaning that it doesn't affect the global random state, but is still reproductible. An example would be (pseudo code):

def test1(): """gs should ensure reproducibility without affecting the global random state.""" tf.set_seed(0) a = tf.random.uniform() gs = tf.random.Generator.from_seed(8448) rrelu_result = rrelu(..., gs=gs) tf.set_seed(0) gs = tf.random.Generator.from_seed(8448) assert rrelu(..., gs=gs) == rrelu_result assert a == tf.random.uniform()

What do you think?

I revert old test cases.

gabrieldemarmiesse

Thanks for the pull request! That should help users who don't want to affect the global random state.

* refact with Generator * merge conflicts * fix test cases * fix license * add type * add return dtype * add optional and remove gradient * change gs is None behavior * fix tests * add optional * revert old test cases Co-authored-by: gabrieldemarmiesse <[email protected]>

fsx950223 requested review from facaiy and seanpmorgan as code owners January 17, 2020 06:30

boring-cyborg bot added the activations label Jan 17, 2020

googlebot added the cla: yes label Jan 17, 2020

WindQAQ self-requested a review January 18, 2020 06:59

fsx950223 force-pushed the rrelu-refact branch from 4f4dc8c to de82812 Compare February 8, 2020 05:30

googlebot added cla: no and removed cla: yes labels Feb 26, 2020

googlebot added cla: yes and removed cla: no labels Feb 26, 2020

gabrieldemarmiesse reviewed Mar 6, 2020

View reviewed changes

gabrieldemarmiesse self-assigned this Mar 6, 2020

gabrieldemarmiesse added the blocked Pending something elses completion label Mar 6, 2020

gabrieldemarmiesse mentioned this pull request Apr 26, 2020

Dropping support for tensorflow < 2.2. #1734

Closed

seanpmorgan removed the blocked Pending something elses completion label Apr 26, 2020

fsx950223 added 2 commits April 27, 2020 10:30

refact with Generator

d783ad9

merge conflicts

aa852db

fsx950223 force-pushed the rrelu-refact branch from 8bb1846 to aa852db Compare April 27, 2020 03:27

fsx950223 added 4 commits April 27, 2020 03:30

fix test cases

57a07d1

fix license

cc51843

add type

ca3e6ba

add return dtype

28c1189

gabrieldemarmiesse reviewed May 2, 2020

View reviewed changes

add optional and remove gradient

6a302bf

gabrieldemarmiesse reviewed May 2, 2020

View reviewed changes

change gs is None behavior

9da8f64

fix tests

ee41e6c

add optional

538b3bd

gabrieldemarmiesse reviewed May 7, 2020

View reviewed changes

fsx950223 and others added 2 commits May 8, 2020 07:49

revert old test cases

828846e

Merge branch 'master' into fsx950223_rrelu-refact

e321f9e

fsx950223 requested a review from gabrieldemarmiesse May 9, 2020 08:51

gabrieldemarmiesse approved these changes May 9, 2020

View reviewed changes

gabrieldemarmiesse merged commit e244936 into tensorflow:master May 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refact with Generator #905

refact with Generator #905

fsx950223 commented Jan 17, 2020 •

edited

Loading

googlebot commented Feb 26, 2020

gabrieldemarmiesse commented Feb 26, 2020

googlebot commented Feb 26, 2020

gabrieldemarmiesse commented Feb 26, 2020

gabrieldemarmiesse left a comment

gabrieldemarmiesse Mar 6, 2020

fsx950223 Mar 6, 2020

bhack Mar 30, 2020

seanpmorgan Apr 26, 2020

gabrieldemarmiesse left a comment

gabrieldemarmiesse May 2, 2020

gabrieldemarmiesse May 2, 2020

fsx950223 May 2, 2020 •

edited

Loading

gabrieldemarmiesse May 2, 2020

fsx950223 May 2, 2020

gabrieldemarmiesse left a comment

gabrieldemarmiesse May 2, 2020

gabrieldemarmiesse May 2, 2020

fsx950223 May 6, 2020

fsx950223 commented May 7, 2020

gabrieldemarmiesse left a comment

gabrieldemarmiesse May 7, 2020

gabrieldemarmiesse May 7, 2020

gabrieldemarmiesse May 7, 2020

gabrieldemarmiesse May 7, 2020

gabrieldemarmiesse May 7, 2020

fsx950223 May 8, 2020

gabrieldemarmiesse left a comment

	gs: tf.random.Generator = None,
	gs: Optional[tf.random.Generator] = None,

		raise ValueError("Either seed or gs should be specific")
		alpha = gs.uniform(tf.shape(x), minval=lower, maxval=upper, dtype=x.dtype)

		@@ -23,43 +23,37 @@
		SEED = 111111


		def rrelu_wrapper(lower, upper, training):

refact with Generator #905

refact with Generator #905

Conversation

fsx950223 commented Jan 17, 2020 • edited Loading

googlebot commented Feb 26, 2020

gabrieldemarmiesse commented Feb 26, 2020

googlebot commented Feb 26, 2020

gabrieldemarmiesse commented Feb 26, 2020

gabrieldemarmiesse 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

gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsx950223 May 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldemarmiesse 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

fsx950223 commented May 7, 2020

gabrieldemarmiesse 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

gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

fsx950223 commented Jan 17, 2020 •

edited

Loading

fsx950223 May 2, 2020 •

edited

Loading