search

klabhub / neurostim

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

Two problems with adaptive parameters #121

Closed adammorrissirrommada closed 4 years ago

adammorrissirrommada commented 5 years ago

1) I just fixed a bug related to the use of adaptive parameters. Any property that is set to be an adaptive plugin must be de-referenced through use of uplus() +myPlg.myProp (otherwise, the handle to the adaptive plugin is returned rather than it's value). So, this commit fixes this example, but we basically never use uplus() in front of assignments throughout the whole code-base, so there must be countless other examples that will cause an error or strange behaviour when someone tries to jitter a hitherto unjittered property.

2) Adaptive values are updated in afterTrial(). This seems a little odd. Why is it in afterTrial rather than beforeTrial? I think the answer is because for many adaptive plugins (e.g. QUEST), the value is updated based on whether the just-completed trial was correct or incorrect. Fair enough. But this means that the classic ('atTrialTime', Inf) argument to parameter.get() would return the value for the NEXT trial rather than the one just completed. That requires amazing knowledge of the inner workings of ns during the analysis! Similarly, any plugin that has an afterTrial() function that is executed AFTER the adaptive one (based on c.order), but draws on the adapted value, would operate on the just-set value for the impending trial rather than the one used for the current trial. Again, you'd really need to know ns well to know that you had to put the adaptive plugins last.

@bartkrekelberg - any thoughts on this? I think there are potentially some existing data-sets that are being analysed with the wrong values through use of 'atTrialTime', Inf

bartkrekelberg commented 5 years ago

  1. Yes that is a downside. I don't think it occurs as frequently as you think (often jitters are added to something or multiplied, in which case there is no problem). Also, where x is used when a +x is needed, an error will occur (double expected , adaptive parm received) -so these errors won't be hidden for long (yes it would be nice to generate a nice warning message when this occurs, but I cannot think of a way to code that).

  2. I agree this is a problem. We could just move the update to beforeTrial() ...? We'd have to exclude running the update in trial==1 and the last trial will never lead to an update, but that is ok. Seems cleaner.

I just checked the adaptiveDemo instead of 'atTrialTime', inf it uses orientation = get(c.grating.prms.orientation,'after','startTime'); I am not sure whether this was intentional or dumb luck, but that solution would not suffer from the "bug" (Still worth fixing though!)

adammorrissirrommada commented 4 years ago

Pushed a potential fix for issue 2 above. See https://github.com/klabhub/neurostim-ptb/commit/6b6e581e5a191411b4ef17c669142adb9d35c483

adammorrissirrommada commented 4 years ago

Are there any other derived adaptive plugins that you know of? I've updated the ones I know.

cnuahs commented 4 years ago

Hmm, @adammorrissirrommada did you just commit a fix? I don't know what's going on here, the link above takes me to a page that says "No results match your search".

adammorrissirrommada commented 4 years ago

Doesn't work for me either actually. ANyway, it's in a branch named adaptivefix. Here:

https://github.com/klabhub/neurostim-ptb/tree/adaptivefix

bartkrekelberg commented 4 years ago

This solution looks good, but when running the adaptiveDemo (with 10 repeats) I see a different result in the analysis than before.

I am not sure why this happens. Can @adammorrissirrommada do a direct comparison side-by-side of the old and new method?

adammorrissirrommada commented 4 years ago

It works - gives the same result with new and old code (thresholds of ~0.65 and ~0.15). The default demo is QUEST, which is pretty volatile early on, so 10 trials is really not enough for it to stabilise and get reliable results. That's why I also increased the number of trials in the demo to 50. Perhaps try it again and see if it looks better?

adammorrissirrommada commented 4 years ago

When I made the adaptivefix branch, I inadvertently branched from the one that was the basis for adamtemp, so there were a bunch of redundant changes (i.e. already shown in the adamtemp pull request) not relevant for the adaptive fix. Sorry about that. So I've now merged the adaptive changes into into adamtemp https://github.com/klabhub/neurostim-ptb/tree/adamtemp to remove the ambiguity: that branch contains all the changes that I seek to merge into master.