[DOC] Update docstring for fixed_seed to ref None option #104

emdupre · 2018-08-01T04:03:20Z

Closes #87 .

Changes proposed in this pull request:

Updates the docstring for the fixed_seed argument in eigendecomp.py and tedana.py

codecov · 2018-08-01T04:13:29Z

Codecov Report

Merging #104 into master will decrease coverage by 0.05%.
The diff coverage is 25%.

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
- Coverage   45.42%   45.36%   -0.06%     
==========================================
  Files          28       28              
  Lines        1585     1587       +2     
==========================================
  Hits          720      720              
- Misses        865      867       +2

Impacted Files	Coverage Δ
tedana/workflows/tedana.py	`11.86% <0%> (ø)`	⬆️
tedana/decomposition/eigendecomp.py	`10.92% <0%> (-0.19%)`	⬇️
tedana/utils/io.py	`13.76% <66.66%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c5f93d...aec1915. Read the comment docs.

tsalo

There are just a couple of typos, but mostly looks good.

tsalo · 2018-08-01T04:15:54Z

tedana/workflows/tedana.py

@@ -149,7 +149,9 @@ def _get_parser():
    parser.add_argument('--seed',
                        dest='fixed_seed',
                        type=int,
-                        help='Seeded value for ICA, for reproducibility.',
+                        help=('Value passed to repr(mdp.numx_rand.seed()) '
+                              'Set to an integer value for reproducibile ICA results; '


reproducibile --> reproducible

tsalo · 2018-08-01T04:16:02Z

tedana/workflows/tedana.py

@@ -149,7 +149,9 @@ def _get_parser():
    parser.add_argument('--seed',
                        dest='fixed_seed',
                        type=int,


This will break with None. Also (and this might not matter to anyone), but typing --fixed_seed None is a little awkward. I think I've seen something like --fixed_seed -1 which would then be coerced to None later.

tsalo · 2018-08-01T04:16:52Z

tedana/workflows/tedana.py

@@ -228,7 +230,9 @@ def tedana_workflow(data, tes, mixm=None, ctab=None, manacc=None, strict=False,
    Other Parameters
    ----------------
    fixed_seed : :obj:`int`, optional


:obj:int or :obj:None, I think.

tsalo · 2018-08-01T04:17:02Z

tedana/workflows/tedana.py

@@ -228,7 +230,9 @@ def tedana_workflow(data, tes, mixm=None, ctab=None, manacc=None, strict=False,
    Other Parameters
    ----------------
    fixed_seed : :obj:`int`, optional
-        Seeded value for ICA, for reproducibility.
+        Value passed to ``mdp.numx_rand.seed()``.
+        Set to an integer value for reproducibile ICA results;


Same typo as above.

tsalo · 2018-08-01T04:18:45Z

tedana/decomposition/eigendecomp.py

@@ -260,7 +260,9 @@ def tedica(n_components, dd, conv, fixed_seed, cost, final_cost,
    conv : :obj:`float`
        Convergence limit for ICA
    fixed_seed : :obj:`int`


:obj:int or :obj:None

tsalo · 2018-08-01T04:18:54Z

tedana/decomposition/eigendecomp.py

@@ -260,7 +260,9 @@ def tedica(n_components, dd, conv, fixed_seed, cost, final_cost,
    conv : :obj:`float`
        Convergence limit for ICA
    fixed_seed : :obj:`int`
-        Seed for ensuring reproducibility of ICA results
+        Value passed to ``mdp.numx_rand.seed()``.
+        Set to an integer value for reproducibile ICA results;


reproducibile --> reproducible

emdupre · 2018-08-01T04:29:36Z

I cheated and fixed the typos with an git commit --amend 😆

The awkwardness of fixed_seed=None is a good point, though. Do you think it's too much ? We could do a couple of things, in my mind:

Have None be the default and specify that users should set a seed
Go ahead with the dual :obj:int or :obj:None identity, as you suggested
Try for a special value like -1, but I am worried that could actually be a valid seed

WDYT ? Do you have a preference here ?

tsalo · 2018-08-01T04:38:14Z

Smart!

Regarding the fixed_seed values- I know that -1 breaks numpy.random.seed, so I don't expect it to be a valid value. I do think that None should be the default, although that will involve changing the order of the arguments in tedica, but that will still involve changing the docstrings. I don't know if setting the default value in the argument parser to None will conflict with the type being int, but my intuition is that it won't be a problem (unlike feeding in --seed None directly).

emdupre · 2018-08-01T04:48:46Z

I just checked with scipy (what this is really a wrapper for), and -1 also breaks, so we could coerce the value. I think the choice then comes down to whether or not None should be the default.

I'm not enthusiastic about it, since I'd like to have results be reproducible out of the box unless a user knows and wants to specifically request non-replicable results. Happy to hear other thoughts, though ! Could you explain why you think None should be the default ?

emdupre · 2018-08-01T15:52:00Z

@KirstieJane might have an opinion here, too, since her group also implemented a fixed_seed argument !

KirstieJane · 2018-08-01T16:01:34Z

To be clear, the question is which should be the default, right? None or an integer value.

I can see both sides. There's a danger that groups won't mix up the random seed and therefore every run will be weirdly similar.....but I think the bigger danger is that people don't fix the random seed and every time they re-run the code they get (slightly) different answers.

So, my not-very-strong vote is for the default to be an integer rather than None....but with clearly documentation to say - go ahead and change this value as you choose!

handwerkerd · 2018-08-01T18:14:13Z

The changes look fine to me. I'm fine merging, as is, but I'll make some comments in response to other's concerns regarding the default option and the awkwardness of --fixed_seed=None

Earlier, I agreed that the default should be a fixed seed so that the default is replicable results, but understand why default randomness could be useful. If that's the general consensus, for replicability, we'd just need to save the random seed somewhere in the output. The easiest way to do this would be to use a random number generator to assign a value to fixed_seed and then make sure to save that number in the output.
For example, replace

tedana/tedana/decomposition/eigendecomp.py

Line 284 in 0c5f93d

mdp.numx_rand.seed(fixed_seed)

with

if fixed_seed == None:
    fixed_seed=np.random_integers(low=1)
mdp.numx_rand.seed(fixed_seed)

and then make sure that fixed_seed is actually part of the output of the code. I'd recommend this in general because, even if people use None, they might want to replicate a specific iteration at some point in the future.

KirstieJane · 2018-08-01T21:26:04Z

Endorse @handwerkerd's suggestion, even if we do keep a fixed seed as the default! 💯

emdupre · 2018-08-02T19:03:28Z

Ok, I've edited this to take -1 as the value for non-reproducible calls, but following @handwerkerd's suggestion I'm also writing out the seed that was actually used !

Happy to hear any other suggestions, here. And I'd always appreciate another review, @tsalo :)

emdupre · 2018-08-04T03:04:37Z

Merging !

emdupre mentioned this pull request Aug 1, 2018

seed for ICA is never random #87

Closed

emdupre requested a review from tsalo August 1, 2018 04:05

emdupre force-pushed the ica-seed branch from 61d4b1c to 79eeeab Compare August 1, 2018 04:13

emdupre force-pushed the ica-seed branch from 79eeeab to 8f1186a Compare August 1, 2018 04:14

tsalo requested changes Aug 1, 2018

View reviewed changes

emdupre force-pushed the ica-seed branch from 8f1186a to 39ee2a5 Compare August 1, 2018 04:24

Update docstring for fixed_seed to ref None option

31ee3c7

emdupre force-pushed the ica-seed branch from 39ee2a5 to 31ee3c7 Compare August 1, 2018 04:26

Write out seed value for ICA

e53f94a

emdupre force-pushed the ica-seed branch from e90b043 to e53f94a Compare August 2, 2018 19:01

Docstring for n_vols in writeresults

aec1915

emdupre merged commit e0ee1cc into ME-ICA:master Aug 4, 2018

emdupre deleted the ica-seed branch August 20, 2018 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DOC] Update docstring for fixed_seed to ref None option #104

[DOC] Update docstring for fixed_seed to ref None option #104

emdupre commented Aug 1, 2018

codecov bot commented Aug 1, 2018 •

edited

Loading

tsalo left a comment

tsalo Aug 1, 2018

tsalo Aug 1, 2018

tsalo Aug 1, 2018

tsalo Aug 1, 2018

tsalo Aug 1, 2018

tsalo Aug 1, 2018

emdupre commented Aug 1, 2018

tsalo commented Aug 1, 2018

emdupre commented Aug 1, 2018

emdupre commented Aug 1, 2018

KirstieJane commented Aug 1, 2018

handwerkerd commented Aug 1, 2018

KirstieJane commented Aug 1, 2018

emdupre commented Aug 2, 2018

emdupre commented Aug 4, 2018

[DOC] Update docstring for fixed_seed to ref None option #104

[DOC] Update docstring for fixed_seed to ref None option #104

Conversation

emdupre commented Aug 1, 2018

codecov bot commented Aug 1, 2018 • edited Loading

Codecov Report

tsalo left a comment

Choose a reason for hiding this comment

tsalo Aug 1, 2018

Choose a reason for hiding this comment

tsalo Aug 1, 2018

Choose a reason for hiding this comment

tsalo Aug 1, 2018

Choose a reason for hiding this comment

tsalo Aug 1, 2018

Choose a reason for hiding this comment

tsalo Aug 1, 2018

Choose a reason for hiding this comment

tsalo Aug 1, 2018

Choose a reason for hiding this comment

emdupre commented Aug 1, 2018

tsalo commented Aug 1, 2018

emdupre commented Aug 1, 2018

emdupre commented Aug 1, 2018

KirstieJane commented Aug 1, 2018

handwerkerd commented Aug 1, 2018

KirstieJane commented Aug 1, 2018

emdupre commented Aug 2, 2018

emdupre commented Aug 4, 2018

codecov bot commented Aug 1, 2018 •

edited

Loading