search

klabhub / neurostim

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

Improved ASyncFlip , Updated Eyelink , remote logger #123

Closed bartkrekelberg closed 4 years ago

bartkrekelberg commented 5 years ago

Major changes:

  1. Synchronous call to FLIP, but always execute on the next VBL. This has the disadvantage that some time is (potentially) spent waiting for the VBL. If this mode works well for you, use it.

  2. Asynchronous call to FLIP: schedule the flip, then continue and flip in the background. At the moment this "extra time" is used to do afterFrame processing. On my system this really improves performance (zero drops now where we had 10-20% before.). Currently , the disadvantage is that the afterFrame code executes (potentially) before the frame is actually visible (Not a big deal because there should be no drawing code in afterFrame anyway), but also before the flipCallbacks have happened, which means that startTime, stopTime may not have been set yet. My stimuli don't depend on that, I think, but some could.

This could be fixed relatively easily by running the flipCallbacks earlier (before the afterFrame call in cic.run) and use the predicted flip time in those calls. In a properly functioning system those predicted flipTimes should be the same as the actual flip times. If the frame drops then they are obviously wrong, but then they would have been wrong in the previous versions too. So I don't think we'd lose anything, but we do gain speed. Comments?

Smalller fixes

cnuahs commented 5 years ago

Oh man, so much stuff here! I'll try merge/test this branch on my two rigs this week...

cnuahs commented 5 years ago

@bartkrekelberg, running behaviorDemo() produces the following error for me:

Argument to dynamic structure reference must evaluate to a valid field name.

Error in neurostim.plugin/localizeParms (line 560)
                    srcParameters{prm} = o.prms.(src); %#ok<AGROW>

this seems related to the localized variables (i.e., 'loc_X') introduced in commit bbd62638.

On my system, the call to extractAfter() on line 559 of plugin.m:

src = extractAfter(targetMembers{prm},'loc_');

returns a string object:

K>> src

src = 

  string

    "X"

K>>

which causes the dynamic structure reference on the following line to fail, seemingly requiring a char vector, not a string.

cnuahs commented 5 years ago

Hmm. This looks like a change in behaviour from version 2016b (that I am running) to 2017a (and presumably later versions). Apparently, as of R2017a, the output data type depends on the type of the first input argument whereas in 2016b extractAfter() always returned string arrays.

So, a couple of options here I guess:

  1. mandate that neurostim requires >2017a, or
  2. explicitly cast the output of extractAfter() to char().

Any thoughts?

bartkrekelberg commented 5 years ago

An explicit cast seems simple and most elegant ? (Presumably char('X') simply returns 'X', so no harm done?)

Bart Krekelberg, PhD

Professor, Rutgers University - Newark

Co-Director, Center for Molecular and Behavioral Neuroscience

Associate Director, Rutgers University Brain Imaging Center

197 University Avenue

Newark, NJ 07102

T: +1 551 285 9265

E: bart@rutgers.edu

W: vision.rutgers.edu

Skype: bartkrekelberg

From: Shaun Cloherty notifications@github.com Sent: Monday, May 13, 2019 9:50 To: klabhub/neurostim-ptb Cc: Bart Krekelberg; Mention Subject: Re: [klabhub/neurostim-ptb] Improved ASyncFlip , Updated Eyelink , remote logger (#123)

Hmm. This is looks like a change in behaviour from version 2016b (that I am running) to 2017a (and presumably later versions). Apparently, as of R2017a, the output data type depends on the type of the first input argument whereas in 2016b extractAfter() always returned string arrays.

So, a couple of options here I guess:

  1. mandate that neurostim requires >2017a, or
  2. explicitly cast the output of extractAfter() to char().

Any thoughts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fklabhub%2Fneurostim-ptb%2Fpull%2F123%23issuecomment-491713817&data=02%7C01%7Cbart%40rutgers.edu%7Ce8b577fe348142902ea308d6d777afdc%7Cb92d2b234d35447093ff69aca6632ffe%7C1%7C0%7C636933306393123528&sdata=qm0I681O4r4DCsAVEmQ9i%2BrhTscPPJAAG7LCi%2F2qeA8%3D&reserved=0, or mute the threadhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAC3MRU62CTDJ5UKZAP3G573PVEMUFANCNFSM4HG4DPOQ&data=02%7C01%7Cbart%40rutgers.edu%7Ce8b577fe348142902ea308d6d777afdc%7Cb92d2b234d35447093ff69aca6632ffe%7C1%7C0%7C636933306393133537&sdata=naQBPjlpsbepMnFkvuTJGTUEfu%2BpPkTJC%2BDU0csHRwE%3D&reserved=0.

cnuahs commented 5 years ago

Casting the output of extractAfter() to char() seems fine on my machine (R2016b). However, note that extractAfter() was introduced in R2016b, so plugin.localizeParms() will fail on earlier versions anyway. At present, @cic requires Matlab to be >R2015b... are we willing to bump that minimum release requirement to R2016b?

bartkrekelberg commented 5 years ago

Fine with me, but we could also replace the extractAfter with a regexp to keep more backward compatibility.

Bart Krekelberg, PhD

Professor, Rutgers University - Newark

Co-Director, Center for Molecular and Behavioral Neuroscience

Associate Director, Rutgers University Brain Imaging Center

197 University Avenue

Newark, NJ 07102

T: +1 551 285 9265

E: bart@rutgers.edu

W: vision.rutgers.edu

Skype: bartkrekelberg

From: Shaun Cloherty notifications@github.com Sent: Monday, May 13, 2019 15:47 To: klabhub/neurostim-ptb Cc: Bart Krekelberg; Mention Subject: Re: [klabhub/neurostim-ptb] Improved ASyncFlip , Updated Eyelink , remote logger (#123)

Casting the output of extractAfter() to char() seems fine on my machine (R2016b). However, note that extractAfter() was introduced in R2016b, so plugin.localizeParms() will fail on earlier versions anyway. At present, @cichttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcic&data=02%7C01%7Cbart%40rutgers.edu%7Cc7161999c63841537a9a08d6d7a99e79%7Cb92d2b234d35447093ff69aca6632ffe%7C1%7C0%7C636933520842200751&sdata=nR%2FuwM4V1jwA0scTLU%2FlJbg2K6BBrTBieNawKur4owQ%3D&reserved=0 requires Matlab to be >R2015b... are we willing to bump that minimum release requirement to R2016b?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fklabhub%2Fneurostim-ptb%2Fpull%2F123%3Femail_source%3Dnotifications%26email_token%3DAC3MRU27VRNIXCECQDKO2J3PVFWQXA5CNFSM4HG4DPO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVILRKI%23issuecomment-491829417&data=02%7C01%7Cbart%40rutgers.edu%7Cc7161999c63841537a9a08d6d7a99e79%7Cb92d2b234d35447093ff69aca6632ffe%7C1%7C0%7C636933520842200751&sdata=tshFhLuTviFtEH75KT%2BAa9FymqKMjFkqiuIX4wLWgfk%3D&reserved=0, or mute the threadhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAC3MRUYW33L5XXWYWF3BKQDPVFWQXANCNFSM4HG4DPOQ&data=02%7C01%7Cbart%40rutgers.edu%7Cc7161999c63841537a9a08d6d7a99e79%7Cb92d2b234d35447093ff69aca6632ffe%7C1%7C0%7C636933520842210769&sdata=gkFx5GBS7oE%2F%2Fp8ODFOKLuuqkfwGhTVrS2kwzr9QsYo%3D&reserved=0.

bartkrekelberg commented 5 years ago

@adammorrissirrommada can you please have a look at this too. It is a big change, and a big improvement in performance.

adammorrissirrommada commented 5 years ago

Hi Bart. Yep, sorry for my sluggishness. I dropped the ball on ns, then got overwhelmed by the number of changes in this mega-branch! I did look into the code for the loc_vars yesterday, and it seemed OK. A first major issue is that ns in this branch is broken for me (see “logger” section below). e.g., behaviorDemo doesn't run.

Local copies of parameters 1) It seems unnecessary and impossible for a developer to flag changesInTrial ahead of time. Can't these just be detected at runtime with the checks you added? Because of eScripts, and the actions of other plugins, any parameter can change at any time. There's no way to know. It currently issues a warning if it unexpectedly encounters a change mid-trial, and points the user to modify the addProperty() call. But that isn't really a good solution if this is the only experiment ever in which that property is changed mid-trial, and eventually, all properties would find themselves hard-coded as changing.

2) If I understand it correctly, once a parameter is identified as changeinTrial, it stays that way for the whole experiment. Is that necessary? Seems that the list should be wiped each trial and checked again. The number of changeables across an experiment could be very large, but maybe only one or two are changing per condition That would mean missing out on some of the loc_var benefit.

New "logger" tool 1) This looks amazing. A great addition. But it breaks ns on my system: it relies on the tcpip class, which is part of the Control Toolbox. I can get it, obviously, but this change makes ns dependent on that toolbox, even if the logger is not being used. Is there a reason it is built into cic and obligatory rather than a plugin?

2) Can we name it something other than "logger"? Logging has been used to refer to storing param values in CIC. It simplifies our life if seeing the word "log" in the code refers to one thing only. Maybe your new tool is a "messenger"?

Branches As suggested by the above comments, we need to get better at putting unrelated changes/features into separate branches. That way we can draw down each bit, test it in isolation from other changes, and merge working parts into master immediately. I am no good at doing so - definitely speaking from the comfort of a glass house - but I felt the need for it here. Processing pull requests is easier if we can take a bite-size piece, see all the relevant code changes in one place, and then test it. This branch is a beast!

bartkrekelberg commented 5 years ago

Logger is now changed to messenger and the unnecessary dependence on tcpip is removed.

Local copies For stimuli that are created for a single project, there is not much risk that all variables change in a trial and the developer will know what changes and what will never change in a trial. This was the main usage I had in mind.

For stimuli that are reused by many users your scenario makes sense and yes, it is theoretically possible that every parm would eventually be hard-coded as "changesInTrial" . So the instruction to go in and change the stimulus definition is not the best - I changed it to tell the user to use the setChangesInTrial() function that achieves the same at runtime. Every specific experiment would likely only have to add one or two variables to the changesInTrial list. But note that this is just to eke out the last bit of performance. Even if the user does not do this and has some script that changes var within the trial, it will work fine.

As to auto-detecting - I don't see how this could work. It is easy to detect parameters that change in a trial and adjust those, but detecting which ones do not change in the upcoming trial is not possible (without parsing the code...). And in any case, if the developer did not write the code to use loc_ vars then this would not help with performance. In my view, the developer of a stimulus is in the best position to determine which parms are likely to not change, and if high performance is needed, adjust the beforeFrame code. Future users of the code who change a parameter that the developer thought would be constant can simply ignore the warning, or add a setChangesInTrial to their script.

Wiping the slate clean at the start of a trial is possible (Although it would require a loop through all plugins and parameters, so may take some time), but I don't see this as an improvement. Detecting that it changes in a trial takes some extra time, so wiping does come with some cost during the trial as well. So there is a tradeoff between wiping before each trial and just detecting the change once in an experiment. For some unique paradigms there could be some advantage to having a parameter "changesInTrial" in some trials but not in others, but only the end-user will know when that is the case.

The simple rule is that if performance is fine, ignore the warnings about changesInTrial, if it is not add setChangesInTrial for that experiment.

Branches When I created this pull request (March) the number of changes was a bit more manageable. I forgot that a pull request is for a branch and not for a snapshot of a branch - so all changes since March were added to this pull-request (I intended to do a pull-request later for the loc_ vars ).

I'll see if I can do this differently,but creating multiple branches for pull requests with reviews that take several months cannot work, as I would have to then go back and figure out how to merge them all together (for instance, the blockTree branch/pull-request has been hanging since Feb and to merge it now will require significant effort to make sure that it does not mess up other changes). So I'll pass the ball back: quicker reviews will lead to smaller code changes in each pull request.

adammorrissirrommada commented 5 years ago

Async flips

This could be fixed relatively easily by running the flipCallbacks earlier (before the afterFrame call in cic.run) and use the predicted flip time in those calls. In a properly functioning system those predicted flipTimes should be the same as the actual flip times. If the frame drops then they are obviously wrong, but then they would have been wrong in the previous versions too. So I don't think we'd lose anything, but we do gain speed. Comments?

As I understand it, the flip callbacks currently log the actual onset time, which would include any delay caused by a dropped frame. So I don't think they are wrong in the current version?

Using the predicted flip time might work though. Some things to consider:

  1. We would need to re-locate the s.onsetFunction() call, currently embedded within the afterFlip() logging call. An onset-locked TTL, for example, should only happen after the actual flip.
  2. If using async doesn't completely eliminate frame drops, the predicted values will be wrong (as you say). One possibility is to log the start time a second time, with the correct value, but I am not sure what the flow-on effects of that would be. That would also add even more overhead to an already failing system.
  3. If using async and still getting frame drops, maybe we should just issue a dire warning at the end of the experiment that all bets are off.
adammorrissirrommada commented 5 years ago

Local copies

I'm not understanding something here. It seems to me that it is already wiping the list each trial, with the line: o.trialDynamicPrms = cell(2,0); It then re-builds the list when they are detected during the setting of defaullt/condition parameter values beforeTrial (for string functions and adaptive objs).

If I have that right, your suggestion of calling setChangesInTrial() would not work, unless it was called every trial.

But, also, I don't understand what you mean by:

It is easy to detect parameters that change in a trial and adjust those, but detecting which ones do not change in the upcoming trial is not possible

If the list is wiped, you assume all do not need to be updated (except those that are auto-detected beforeTrial), until you encounter one that does, then add it to the list. There is some overhead in making that detection, but that happens only once per trial per unexpected parameter change. That'd be OK I think (given the boost of the loc_ approach is quite large anyway)?

Ultimately, what I am saying is that, if possible, I'd prefer a solution that doesn't require the user to know about setChangesInTrial() at all. Those approaches include wipe-clean-and-auto-detect or retain-all-and-auto-detect.

adammorrissirrommada commented 5 years ago

(oh, I think it is retaining the list after all... parameters themselves remember that they are changing and get re-added after it gets wiped)

bartkrekelberg commented 5 years ago

Ultimately, what I am saying is that, if possible, I'd prefer a solution that doesn't require the user to know about setChangesInTrial() at all. Those approaches include wipe-clean-and-auto-detect or retain-all-and-auto-detect.

if the cost of detecting a change is low, then everything could be done automatic and behind the scenes (i.e. without ever using changesInTrial flags in addProperty or by a function call) and we'd never even issue a warning. The developer would just define and use loc_ vars and the rest would be automatic. Resetting the flag after each trial would add additional overhead in the intertrial interval.

This seems possible, but given that users can already ignore the warning without risk, the current solution is not that different. If anyone wants to spend the time to benchmark the fully automatic version that is fine with me.

But note that all of this only applies to stimuli where the developer has chosen to use loc_ vars and has written the beforeFrame functions accordingly. Most current stimuli are not affected at all by this.

adammorrissirrommada commented 4 years ago

@bartkrekelberg and @cnuahs I am trying to merge this branch into master on my machine to test it, but the fixate.m conflict is tripping me up. I think it has changes from each of you, and it's not clear to me which parts to keep/ditch?