seed for ICA is never random

[handwerkerd]

I noticed that the seed for the ICA step is always defined
Default set in 2 ways:

tedana/tedana/cli/run_tedana.py

Line 154 in 2fa0136

default=42)

tedana/tedana/workflows/tedana.py

Line 32 in 2fa0136

fixed_seed=42, debug=False, quiet=False):

The random number generator is seeded based on the default or a user defined seed:

tedana/tedana/decomposition/eigendecomp.py

Line 262 in 2fa0136

mdp.numx_rand.seed(fixed_seed)

There's actually no way to run the code with a randomly assigned seed. I'm not sure this is a good or bad design choice, but it's not clearly documented that the seed is always fixed. (I had assumed it would be random unless I set the variable). I think it does make sense to set the default to a fixed seed, but I could imagine situations where someone might want a random seed. Whether or not an unseeded option is added, the fact that the code defaults to a fixed seed should be better noted.

I can take care of this, but I wanted to make sure I wasn't missing something first & get feedback on whether it's worth added a non-fixed seed option.

[emdupre]

Thanks for bringing this up, @handwerkerd !

Yes, my thinking is that the default should be to run with a set seed to encourage reproducible results (which I think most users will expect, particularly new users). What we could do is allow people to explicitly pass None for the fixed_seed argument, and then treat that as unseeded.

Right now this argument is documented in the usage section— do you mean that we should expand on the docstring for that argument, or note it somewhere else, additionally ?

[handwerkerd]

The language could probably be tweaked a bit there & in the output to "tedana --help" to make clear that it is always fixed. For example, "Initial rand num generator is always fixed so ICA results are reproducible."

Adding an option to explicitly pass None might be nice, but isn't essential.

[emdupre]

That sounds great ! I'd be happy to see an explicit None option, too— PR welcome 😄

[emdupre]

Ok, I went and looked through this and I think the current implementation actually does support passing None if results should be allowed to vary between runs.

The default is mdp.numx_rand.seed(42), but that's just a wrapper for scipy.random.seed which does allow for passing None. So the only change we'd need to make is to document that None can be passed -- maybe in the argument description ? Where do you think would be best, @handwerkerd ?

[handwerkerd]

Thank you for looking into this. That makes this a really easy fix.
It probably just needs an added comment like, 'Set to None to use a random seed' here:

tedana/tedana/decomposition/eigendecomp.py

Line 241 in 2fa0136

Seed for ensuring reproducibility of ICA results

Here:

tedana/tedana/workflows/tedana.py

Line 85 in 2fa0136

Seeded value for ICA, for reproducibility.

and here:

tedana/tedana/cli/run_tedana.py

Line 153 in 2fa0136

help='Seeded value for ICA, for reproducibility.',

I don't think this needs to be mentioned anywhere else.

[emdupre]

Just opened #104 to close this. It'd be great to get your review there, @handwerkerd, to make sure it's in line with what you were looking for !