Bug in emGenSequence produces the same eye movement paths for different trials

[elinekupers]

I have a question about the latest version of the default eye movement model generated with 'fixationalEM'.

It seems to me that the eye movement paths that are generated by emGenSequence for different trials are the same, because they have a fixed scalar as the input to the random generator seed. See line 93 in emGenSequence: p.addParameter('rseed', 1, @isscalar);

Since the input parser only accepts scalars, it does not accept the random number generator to be set to 'shuffle'. If you do not define the parameter rSeed (you cannot define it as empty, since that is not a scalar), it will automatically be set to the default parameter (which is 1). See line 103 in emGenSequence: if ~isempty(p.Results.rseed), rng(p.Results.rseed); end

Since this rSeed parameter is set as 1 and inherited by the compute() function, which only uses 'shuffle' when the rSeed parameter is empty (which is never the case), see line 54-59 in compute:

% Set random seed
if (isempty(obj.randomSeed))
    rng('shuffle');
else
    rng(obj.randomSeed);
end

EmGenSequence function will always use the rng(1) and I believe this will always provide the same emPaths every time you run emGenSequence. You can try it yourself with this little script:

cMosaic     = coneMosaic('center', [0.0015  0]);
numEm = 100;
emPaths1 = cMosaic.emGenSequence(numEm, 'nTrials', 5, 'em', fixationalEM);
emPaths2 = cMosaic.emGenSequence(numEm, 'nTrials', 5, 'em', fixationalEM);

isequal(emPaths1, emPaths2)

My question is: Is this a bug? If yes, I think it can easily be fixed by setting the default of rSeed as empty and not forcing it to be a scalar in emGenSequence: p.addParameter('rseed',[])

If no: How can I generate random eye movement paths for different trials of an OIS? I use the latest version of the master branch (06/23/2018)

[wandell]

Good point about the behavior. We should all discuss what behavior we want.

I think what was intended (and this issue arises in multiple places) was that each time you called the routine you got the same behavior. If you wanted a different random sequence, you should send a different seed. The rng is reset on line 103 of emGenSequence.m

if ~isempty(p.Results.rseed), rng(p.Results.rseed); end

So, to get a reset rng, one can send in a scalar value. But if you do nothing, then you will always get the seed at 1.

It's a bit late, and I am not sure what we want. But let's hear from @npcottaris and @DavidBrainard what they think the behavior should be.

[elinekupers]

Ok, yes, that makes sense. I just wonder why it checks if it is empty, because rSeed is not allowed to be empty as an input due to the "@isscalar" requirement at the input parser.

If we allow it to be either empty or a scalar, rng() has the possibility to later be set to 'shuffle'.

Or should I input a different scalar for rSeed every time I run emGenSequence?

[DavidBrainard]

I think the code as written just reflects a buglet, with the intent being that the arg check would allow the empty matrix to be passed.

I replaced the arg check with @(x) (isempty(x) | isscalar(x)) and updated comments that passing [] gets the 'shuffle' behavior.

But, I didn't check that it works. Could someone try it out.

Also, how do we feel about routines changing the rng seed to a fixed value deep down? Should they perhaps get and restore the rng state as they were called? This is a question of coding convention that we should think through.

[elinekupers]

Thanks! I checked your updates and it works.

I am not sure what the best convention is for changing the rng. At least for me, resetting it to a fixed seed caught me off guard (which is my fault for assuming and not checking).

One possibility is to specify it in the cMosaic and let emGenSequence inherit this rSeed setting?

Although I quickly checked and saw that cMosaic uses the option for a fixed rSeed when creating the mosaic pattern. I assume that users don't want to necessarily fix both pattern and emGenSequence rSeeds at the same time with the same value, to that might not be better in the end..

[elinekupers]

For now I'm closing this issue, but please feel free to reopen it again.

[npcottaris]

This is an issue warranting special attention, especially as new processing stages get cascaded and the user interacts with deep isetbio components via high levels scripts. I think that the default behavior should a fixed seed (as Brian suggests), and that any ISETbio component that changes the rng state should save its current state before starting computation, and once finished it should restore that initial state (as David suggests). This should be done consistently with all components that modify the rng state, suggesting that maybe we need a centralized function that manages therng state and that all ISETbio components interact with Matlab's rng, via this function only.

[npcottaris]

Moreover, the proposed centralized function should get passed the rseed argument that the ISETbio component received, and only accept it if belongs to a set of valid arguments. This will enforce consistent usage of the rSeed argument across the entire stack.

[DavidBrainard]

Closing in favor of continuing discussion in separate issue.