Closed bartkrekelberg closed 4 years ago
My feeling is that we could get rid of alwaysOn altogether and not lose any performance (using instead, s.on==0 and s.duration == Inf), with a minor change to the code.
Currently, these are the NS params that are evaluated in that section:
if its false, additionally
The few additional calculations in the else clause are modest and draw only on local variables. (fast)
If we cached s.cic.screen.frameRate (slooow) into a non-NS member variable of s in beforeExperiment() (fast) , there'd be only two ns params evaluated in the latter section. That's the same number that you'll end up with if you add a further check of s.on to the alwaysOn clause.
This would break existing experiments that use alwaysOn, but the fix needed is trivial. If we wanted to smooth the transition, we could make a local, non-ns variable called alwaysOn, and add a set() function that detects it is being used and to issue an error with instructions for fixing it.
(by the way, we should do the caching frame rate bit regardless!)
I agree that the utility of alwaysOn isn't clear to me. One possibility might be to deprecate it going forward, and for backward compatibility add a dependent (non-neurostim) property with get() and set() methods that set .on = 0 and .duration = Inf, when set to true, and check for that condition and return true or false as appropriate when read. That removes any potential ambiguity and provides the ability to run old experiment scripts, but use/checking of alwaysOn could be removed from the stimulus base class code entirely so there is no runtime overhead.
The difference with .duration =inf and .alwaysOn=true is that the latter stays on in the ITI, the former does not.
This is a distinction/functionality we need, so I don't think we can just get rid of .alwaysOn
I thought it was C.itiClear that did that? I'm on my phone, so viewing code a bit challenging, but I can't see anywhere that links alwaysOn to the ITI.
You're right. s.on=0 s.duration =inf c.itiClear = 0
will keep s on during the ITI.
So the plan is
I'll leave this as a todo for when we've merged some older branches.
From: Adam Morris notifications@github.com Sent: Thursday, September 26, 2019 22:02 To: klabhub/neurostim-ptb neurostim-ptb@noreply.github.com Cc: Bart Krekelberg bart@vision.rutgers.edu; Author author@noreply.github.com Subject: Re: [klabhub/neurostim-ptb] AlwaysOn and Startime (#130)
I thought it was C.itiClear that did that? I'm on my phone, so viewing code a bit challenging, but I can't see anywhere that links alwaysOn to the ITI.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fklabhub%2Fneurostim-ptb%2Fissues%2F130%3Femail_source%3Dnotifications%26email_token%3DAC3MRUY6JI7XVZHOMOEBX3LQLUINZA5CNFSM4I2PIWMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7WZODQ%23issuecomment-535664398&data=02%7C01%7Cbart%40rutgers.edu%7C69dc911f323146d5efc608d742bc7ede%7Cb92d2b234d35447093ff69aca6632ffe%7C1%7C0%7C637051249658043075&sdata=%2F2R%2FTwbmckVI%2F1LvbIDSY0VqGeAp7Fpv5KVhg1vqhH4%3D&reserved=0, or mute the threadhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAC3MRU4G2NMXOOC3KV3F2Z3QLUINZANCNFSM4I2PIWMA&data=02%7C01%7Cbart%40rutgers.edu%7C69dc911f323146d5efc608d742bc7ede%7Cb92d2b234d35447093ff69aca6632ffe%7C1%7C0%7C637051249658053067&sdata=S%2Fg1VVVtiQE5QdhmbE9US8wbPon04m8umF0Rr%2B73jeQ%3D&reserved=0.
I removed alwaysOn from stimulus.m I did not cache screen.framerate , probably not necessary.
It is currently possible to specify .alwaysOn ==true, and also .on = something.
The .on will be ignored (overruled by alwaysOn) in the base class.
But the user can have beforeFrame code that uses the .on values. : if trialTime >s.on; drawSomething .
This is confusing at analysis time when .startTime will equal firstFrame and yet the stimulus may only draw later
I think the best way to address this is to check that .on is empty or NaN whenever alwaysOn is true. Setting both is contradictory and never (?) a good idea. So at the very least a warning is in order, and probably an error?