ENH: Set random seeds #101

gwarmstrong · 2019-12-16T19:28:23Z

Songbird random seeds

Current behavior:

When I run:

$ songbird multinomial \
    --input-biom data/redsea/redsea.biom \
    --metadata-file data/redsea/redsea_metadata.txt \
    --formula "1" \
    --epochs 1000 \
    --differential-prior 0.5 \
    --summary-interval 1 \
    --summary-dir logdir/model1
$ songbird multinomial \
    --input-biom data/redsea/redsea.biom \
    --metadata-file data/redsea/redsea_metadata.txt \
    --formula "1" \
    --epochs 1000 \
    --differential-prior 0.5 \
    --summary-interval 1 \
    --summary-dir logdir/model2

I get something like the following, where the cv-error converges to different values and the training error is updated differently:

Further exploration:

So I provided an option to fix the seed for making train/test splits:

$ songbird multinomial \
    --input-biom data/redsea/redsea.biom \
    --metadata-file data/redsea/redsea_metadata.txt \
    --formula "1" \
    --epochs 1000 \
    --differential-prior 0.5 \
    --summary-interval 1 \
    --summary-dir logdir/model3 \
    --seed 42
$ songbird multinomial \
    --input-biom data/redsea/redsea.biom \
    --metadata-file data/redsea/redsea_metadata.txt \
    --formula "1" \
    --epochs 1000 \
    --differential-prior 0.5 \
    --summary-interval 1 \
    --summary-dir logdir/model4 \
    --seed 42

And now the models converge to approximately the same value.
However, as there a still variations in their training updates. So I provided a way to fix the seed that tensorflow uses as well.

$ songbird multinomial \
    --input-biom data/redsea/redsea.biom \
    --metadata-file data/redsea/redsea_metadata.txt \
    --formula "1" \
    --epochs 1000 \
    --differential-prior 0.5 \
    --summary-interval 1 \
    --summary-dir logdir/model5 \
    --split-seed 42 \
    --tf-seed 22
$ songbird multinomial \
    --input-biom data/redsea/redsea.biom \
    --metadata-file data/redsea/redsea_metadata.txt \
    --formula "1" \
    --epochs 1000 \
    --differential-prior 0.5 \
    --summary-interval 1 \
    --summary-dir logdir/model6 \
    --split-seed 42 \
    --tf-seed 22

And now it is identical between runs:

Proposed behavior:

When the above commands are run, I should at least get the same CV score, but furthermore, it should be possible to fix the entire training process.

In order to fix this I propose the following solution:

For QIIME2 plugin:
- Have a fixed random seed for both tensorflow and train/test split
  - this ensures that all results that come out of Q2 for the same data are directly comparable, and the same optimization results are achieved every time
  - additionally removes the need for a user to understand how to appropriately set their on seeds
For the standalone interface:
- Have a random seed option for the train/test split and tensorflow, that are independent of each other
- Set a default seed for train/test split
  - again ensures that results are directly comparable by default
- Have the option to set a seed for tensorflow, but is not set by default
  - This one is less of a big deal since the loss function is convex, but it is nice to be able to set if you want to recreate the exact same process

Summary of changes:

add --split-seed and --tf-seed option to the songbird CLI (and update parameter info accordingly)
add a unit test to scripts.test_songbird_cli that verifies that different combinations of these flags still run on the CLI
Set both random seeds to 0 in the Q2 plugin
add a test to the Q2 plugin that verifies subsequent calls on the same inputs have the exact same output

…ugin

mortonjt

Hi @gwarmstrong , this is awesome.

What do you think about only having 1 seed passed in? I'm assuming that two seeds are defined due to numpy / TF differences. But you can imagine the following should work

def multinomial(..., seed):
    np.random.seed(seed)
    tf.set_random_seed(seed)

It'll be more ideal to only specify 1 seed to try to minimize the number of available options to the users.

mortonjt · 2019-12-17T16:06:01Z

scripts/songbird

@@ -157,6 +181,10 @@ def multinomial(
        save_path=summary_dir,
    )
    with tf.Graph().as_default(), tf.Session() as session:
+        # set the tf random seed
+        if tf_seed is not None:
+            tf.set_random_seed(int(tf_seed))


I'm not sure if the int conversion should be necessary if the type is specified in click.

I think I avoided providing a type originally since it broke convention in the existing decorators. Since you're fine with it, I'll do that instead.

gwarmstrong · 2019-12-17T17:09:42Z

Hi @gwarmstrong , this is awesome.

What do you think about only having 1 seed passed in? I'm assuming that two seeds are defined due to numpy / TF differences. But you can imagine the following should work
def multinomial(..., seed):
    np.random.seed(seed)
    tf.set_random_seed(seed)
It'll be more ideal to only specify 1 seed to try to minimize the number of available options to the users.

Sure, I thought it might be nice to be able to adjust them independently. This could be helpful for, e.g., verifying that you get a somewhat consistent error minimum across different a random initialization, but with the same train/test split.

Given that the loss here is convex with respect to the parameters, my idea is probably less necessary and we can change to a single seed.

gwarmstrong · 2019-12-17T18:17:12Z

@mortonjt this should reflect the changes you suggested now

gwarmstrong · 2020-01-07T22:40:09Z

@mortonjt just a polite bump on this PR! Also noting that this issue has subsequently come up with multiple people within Knight Lab

mortonjt

There are a couple of typos.

If you run this command on a non-zero seed and post your tensorboard results on the example dataset (i.e. redsea) I'll can double check on my machine to make sure those results are reproducible.

mortonjt · 2020-01-08T00:39:50Z

CHANGELOG.md

@@ -1,6 +1,8 @@
 # songbird changelog

 ## Version 1.0.2-dev
+Added ability to set random seed for CLI and sets fixed random seeds for qiime2 []()


want to fix this PR name and number?

mortonjt · 2020-01-08T00:41:26Z

songbird/q2/_method.py

    )

    model = MultRegression(learning_rate=learning_rate, clipnorm=clipnorm,
                           beta_mean=differential_prior,
                           batch_size=batch_size,
                           save_path=None)
    with tf.Graph().as_default(), tf.Session() as session:
+        tf.set_random_seed(0)


Suggested change

tf.set_random_seed(0)

tf.set_random_seed(seed)

this won't work - the seed will always be set to zero otherwise.

Probably will want to have the seed=None option here as well.

For the QIIME plugin, I thought it might be better to make the seed 0 always, so as to avoid exposing a seed argument to the QIIME user. Given this assumption, the code is fine as it stands (tf.set_random_seed(seed) will actually throw a NameError). Thought being that this could cut down on instances of comparing a model run with many different seeds to a baseline that was run only with the default seed, or something like that. It basically enforces the "right" behavior.

If you want me to expose the seed to the user via QIIME, that is fine too, and I can make the corresponding change.

We don't want the seed to be always set to zero in the qiime2 side. Thanks.

Okay. I will rewrite so that default seed for QIIME is 0, but can be set via a parameter.

I'm not sure I understand where seed=None factors into this interface. I think we want the default behavior of the QIIME plugin to be that the result of two commands on the same data are directly comparable by default. Then you can change seed if you need. That shouldn't require a seed=None anywhere for this?

We'll still want to have an option to not specify a seed, namely

if seed is not None: tf.set_random_seed(seed)

songbird/util.py

gwarmstrong · 2020-01-08T01:19:55Z

If you run this command on a non-zero seed and post your tensorboard results on the example dataset (i.e. redsea) I'll can double check on my machine to make sure those results are reproducible.

Does the third tensorboard output in the original description work? Command and tensorboard screenshot is included. Or do you need additional information?

mortonjt · 2020-01-08T01:23:55Z

Yes, the output in the original tensorboard would work - but the interface changed right?

gwarmstrong · 2020-01-08T01:26:10Z

Ah right, that is when the seeds could be set separately. Sure, I will re-run and upload.

gwarmstrong · 2020-01-08T01:33:12Z

Try this on for size

$ songbird multinomial \
    --input-biom data/redsea/redsea.biom \
    --metadata-file data/redsea/redsea_metadata.txt \
    --formula "1" \
    --epochs 1000 \
    --differential-prior 0.5 \
    --summary-interval 1 \
    --summary-dir logdir/new_model \
    --random-seed 42

…gbird into set-random-seeds

mortonjt · 2020-01-08T01:45:50Z

songbird/q2/_method.py

    )

    model = MultRegression(learning_rate=learning_rate, clipnorm=clipnorm,
                           beta_mean=differential_prior,
                           batch_size=batch_size,
                           save_path=None)
    with tf.Graph().as_default(), tf.Session() as session:
+        tf.set_random_seed(0)


We'll still want to have an option to not specify a seed, namely

if seed is not None: tf.set_random_seed(seed)

mortonjt · 2020-01-08T01:46:34Z

I was able to run and reproduce the results

Once the last comment regarding the qiime2 seed is done and verified to work, I can merge this in.

mortonjt

Overall, I'm happy with this PR.

However, I'm still not sure if having a default seed of 0 is suitable in the qiime2 version. The option of not choosing a random seed is already available in the standalone script (so I guess that is addressed).

In theory, I'm ok with these changes, but additional input would be nice. @lisa55asil do you have any thoughts on this?

mortonjt · 2020-01-08T16:11:33Z

songbird/parameter_info.py

@@ -60,4 +64,5 @@
    "checkpoint-interval": 3600,
    "summary-interval": 10,
    "summary-dir": "summarydir",
+    "random-seed": 0,


Suggested change

"random-seed": 0,

"random-seed": None,

I'm still leaning towards having the default be None, otherwise the option to not specify a random seed will no longer exist. For example, if you have a multiple random runs, you do things like take averages, or take the best fit amongst multiple random runs. If this option is not available, it won't be possible for the user to not specify a random seed.

Thanks! And I appreciate the feedback on the PR.

I think it is important to mandate a seed for Q2. For a software that has a mission of reproducibility, you should not be able to create artifacts that you cannot reproduce. If someone were to give me an artifact created by songbird with seed set to None, I’d never be able to reproduce it exactly.

I’m also not sure why you can’t achieve your use case by setting the random seed differently, multiple times? This use case is actually the reason I had two different seeds originally, so you could keep the same train/test split, but change the model fitting. Though this behavior could be achieved by this PR by setting a training column and then varying the seed.

Also, as you mentioned, setting a None seed is still available via the CLI.

mortonjt · 2020-01-08T16:12:23Z

songbird/q2/_method.py

    )

    model = MultRegression(learning_rate=learning_rate, clipnorm=clipnorm,
                           beta_mean=differential_prior,
                           batch_size=batch_size,
                           save_path=None)
    with tf.Graph().as_default(), tf.Session() as session:
+        tf.set_random_seed(random_seed)


Suggested change

tf.set_random_seed(random_seed)

if random_seed is not None:

tf.set_random_seed(random_seed)

In light of the previous comment, it'll be preferable to have this instead

lisa55asil · 2020-01-08T19:19:22Z

I think these changes are great and strongly agree that we should have a default seed set for the q2 plugin. As you mentioned if people want change or remove the seed they can use the standalone version, but it is critical that q2 produces reproducible results.

I'm going to update the readme to make it more clear:

how and why to use the null/baseline model
when to use the --p-training-column

Thanks for the awesome updates George!!

mortonjt · 2020-01-08T19:23:11Z

Thanks @lisa55asil . I'm going to merge this in. @gwarmstrong thanks!

gwarmstrong added 10 commits December 13, 2019 12:28

TST: add checks for consistent results across different runs to q2 pl…

954db17

…ugin

ENH: set a fixed seed for np and tf in q2 plugin

d801861

ENH: add ability to set fixed seed to train/test split

279e595

ENH: add ability to set np and tf seeds in standalone

c42c49c

MAINT: update parameter info with defaults and descriptoins for seeds

7693856

TST: test ability to use seed parameters

24d447e

DEV: add message for random seeds to changelog

8f50ccd

MAINT: remove extra whitespace

c291a6b

MAINT: remove unneccesary import

290002f

BUG: fix error with adding None value to hparams

f976f8a

mortonjt requested changes Dec 17, 2019

View reviewed changes

MAINT: refactored to use only one seed and remove type conversion

fe89c5d

mortonjt requested changes Jan 8, 2020

View reviewed changes

MAINT: add PR version and link to changelog

50e22e0

gwarmstrong added 2 commits January 7, 2020 17:36

ENH: expose random_seed to Q2

fbee7e3

Merge branch 'set-random-seeds' of https://github.com/gwarmstrong/son…

cfb1197

…gbird into set-random-seeds

mortonjt requested changes Jan 8, 2020

View reviewed changes

mortonjt approved these changes Jan 8, 2020

View reviewed changes

mortonjt merged commit 22ec2b5 into biocore:master Jan 8, 2020

gwarmstrong mentioned this pull request Jan 12, 2020

Misaligned summary intervals cause different trajectory plots for identical models #104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Set random seeds #101

ENH: Set random seeds #101

gwarmstrong commented Dec 16, 2019

mortonjt left a comment

mortonjt Dec 17, 2019

gwarmstrong Dec 17, 2019

gwarmstrong commented Dec 17, 2019

gwarmstrong commented Dec 17, 2019

gwarmstrong commented Jan 7, 2020

mortonjt left a comment

mortonjt Jan 8, 2020

gwarmstrong Jan 8, 2020

mortonjt Jan 8, 2020

mortonjt Jan 8, 2020

gwarmstrong Jan 8, 2020

mortonjt Jan 8, 2020

gwarmstrong Jan 8, 2020

mortonjt Jan 8, 2020

gwarmstrong commented Jan 8, 2020

mortonjt commented Jan 8, 2020

gwarmstrong commented Jan 8, 2020

gwarmstrong commented Jan 8, 2020

mortonjt Jan 8, 2020

mortonjt commented Jan 8, 2020

mortonjt left a comment

mortonjt Jan 8, 2020

gwarmstrong Jan 8, 2020 •

edited

Loading

mortonjt Jan 8, 2020

lisa55asil commented Jan 8, 2020

mortonjt commented Jan 8, 2020

	tf.set_random_seed(random_seed)
	if random_seed is not None:
	tf.set_random_seed(random_seed)

ENH: Set random seeds #101

ENH: Set random seeds #101

Conversation

gwarmstrong commented Dec 16, 2019

Songbird random seeds

Current behavior:

Further exploration:

Proposed behavior:

Summary of changes:

mortonjt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwarmstrong commented Dec 17, 2019

gwarmstrong commented Dec 17, 2019

gwarmstrong commented Jan 7, 2020

mortonjt 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

gwarmstrong commented Jan 8, 2020

mortonjt commented Jan 8, 2020

gwarmstrong commented Jan 8, 2020

gwarmstrong commented Jan 8, 2020

Choose a reason for hiding this comment

mortonjt commented Jan 8, 2020

mortonjt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwarmstrong Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lisa55asil commented Jan 8, 2020

mortonjt commented Jan 8, 2020

gwarmstrong Jan 8, 2020 •

edited

Loading