search

klabhub / neurostim

Design and run visual neuroscience experiments using Matlab and the Psychophysics Toolbox.
MIT License
5 stars 3 forks source link

This change allows plugins to be added after calling c.run #164

Closed bartkrekelberg closed 3 years ago

bartkrekelberg commented 3 years ago

This change allows plugins to be added after calling c.run (e.g. in beforeExperiment)

This allows Eyelink to load the sound plugin if it has not been loaded already (and therefore not messing with any sound plugins the user may have loaded already).

Note that c.run still checks that .used == false - so a cic object can still not be reused.

I don't see any downsides (except that a user could start adding plugins afterFrame() which is unlikely to result in a smooth experiment).

There are probably other scenarios where adding a plugin during the experiment would not be crazy....

Alternative solutions are ugly (e.g. would require adding a "forceAdd" option to the plugin constructor, and the plugin.add function).

adammorrissirrommada commented 3 years ago

In the event that a plugin was added midway into an experiment, it's hard to think what the consequences might be for offline analysis. I guess just no param values for early trials. That's fine, as far as I can tell. Another possibility is just to allow add only up to the beforeExperiment() stage and not after.

cnuahs commented 3 years ago

I see only one change here... added an assert() in cic.add(). I can't imagine that being problematic. unless I'm missing something, this seems fine to me.

adammorrissirrommada commented 3 years ago

Shaun, it's a removal of the assert. There is an existing assert in c.run() that checks for re-use of a cic anyway.

bartkrekelberg commented 3 years ago

Seems OK to me, but perhaps adding the sound plugin when needed should have been in the eyetracker.m parent instead (from a different PR, I know)? Many eye trackers would need sound, and even if not, probably no harm in adding it.

I think this points to the real issue; the sound plugin is really something that is rarely used on its own, but rather in support of other plugins. (Tell tale sign is that it doesn't have the usual beforeXX afterXX member functions).

So maybe a better fix would be to make this part of cic (calling it cic.sound would allow for backward compatibility) and some options in cic.hardware could be used to set up this not-plugin and allow users to activate it (or not) for a specific experiment.

Not sure this is worth the time though... but it could be a future modification?

Regardless, I think we should remove the assert as suggested by this PR because it duplicates the assert in run (and has the side effect of not allowing plugins to be added at run time. Because there is nothing inherently wrong with that, I think it should be allowed).

cnuahs commented 3 years ago

Ah, I see. I read the diff backward. All this seems fine to me.