klabhub / neurostim

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

Diode #149

Closed bartkrekelberg closed 4 years ago

bartkrekelberg commented 4 years ago

The square flashed to trigger a diode is a useful way to check timing, but the current code has issues because the flash is yoked to the stimulus in its duration, and its persistence across the ITI.

This causes problems in an experiment with a high background luminance, which would trigger the diode during the ITI.

To address this I split the diodeFlasher off into its own stimulus class that is always linked to the on or offset of a single target stimulus.

help diodeFlasher shows usage and explains some caveats. tools/diodeTimingTester shows an example. Usage is pretty much the same as before.

*I also removed the orphaned mccChannel and userData property (and their loc_ versions) from the stimulus class - I could not find any use of these anywhere in the code (I think we decided removing them before)

adammorrissirrommada commented 4 years ago
adammorrissirrommada commented 4 years ago

This pull request is to pull into gui branch, but if/when we are happy with this, shouldn't it go into master and not gui? i.e. we want gui things to be separately mergeable into master later. If we merge diode into master, you can then pull those changes into gui.

bartkrekelberg commented 4 years ago
  • The defaultOrder to put the flasher last makes sense, but it would never make sense for someone to manually put the flasher before its yoked stimulus. Manual setting of flasher position in c.order() should be forbidden?

True.

  • Perhaps "whenoff" should be "invert", given that it is conceptually similar to the invert functionality of behaviours?
  • Is there any need for stimIsOn to be a ns parameter and logged? Seems unnecessary overhead to log it, and check it each frame.

We can probably do without the logging, given that the targetStimulus will already log its onset.

  • It seems clunky that itiClear=false is the mechanism to ensure that the flasher box continues through the ITI. itiClear needs to be controlled independently for a different purpose. I have been thinking for a while that there should perhaps be a beforeITIframe() function in plugin.m that gets called during the ITI frame counter loop. Perhaps that is a better solution? this plugin could continue drawing. For regular stimuli, but not this one, the opengl transformations would need to be applied in a base() call just like before in-trial frames. If that was implemented, the default behaviour of the flasher could be to use black and white as the low and high colours and to be on 100% of the time (i.e. the background is never visible at that location).

I like this idea. It is a lot cleaner, and allows more flexibility for other stimuli as well. I'll add a beforeITI function.

  • Is there any scenario in which this divorcing of stimulus and flasher drawing commands could mean they are no longer physically coincident on the display? e.g. if frames are not synced to the vertical blank. I suspect your response will be that in that case, your system is just faulty and you shouldn't be using it. I agree. So is there any other scenario you can think of?

No, All beforeFrame code executes "together" before 'Flip' is called. So if the draw code takes too long, the monitor will just show all "previous" frames for another frame.

I need to merge this in gui, as I've already merged gui into our active branch.

cnuahs commented 4 years ago

Having now sunk a couple of hours into using this I have a couple of thoughts here:

  1. adding the diode square is no longer a "zero cost" prospect. Previously, turning on the diode incurred next to no overhead in beforeFrame, but now we incur the entire overhead of a stimulus. It may be that the loc_ variable optimization added earlier reduces this overhead, I am yet to evaluate that.

  2. Adding the diode now messes with itiClear... so adding the diode square causes all stimuli to persist across the iti (since the default for the stimulus duration property is Inf). That means I need to explicitly set the duration of all my other stimuli if I add the diode... I guess that is ok but seems like a lot of busy work. Maybe this would be addressed through addition of the mooted beforeITIFrame() hook...?

bartkrekelberg commented 4 years ago
  1. adding the diode square is no longer a "zero cost" prospect. Previously, turning on the diode incurred next to no overhead in beforeFrame, but now we incur the entire overhead of a stimulus. It may be that the loc_ variable optimization added earlier reduces this overhead, I am yet to evaluate that.

Yes there is additional overhead, but it creates functionality that I could not get to work otherwise (i.e. a diode flash that persists across the ITI while its corresponding stimulus does not; this is needed when the background could trigger the diode).

But the overhead should be small; did you actually see drops or just the potential for framedrops?

  1. Adding the diode now messes with itiClear... so adding the diode square causes all stimuli to persist across the iti (since the default for the stimulus duration property is Inf). That means I need to explicitly set the duration of all my other stimuli if I add the diode... I guess that is ok but seems like a lot of busy work. Maybe this would be addressed through addition of the mooted beforeITIFrame() hook...?

I agree that is messy and needs a better solution. One stimulus should not mess with the others.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHubhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fklabhub%2Fneurostim-ptb%2Fpull%2F149%23issuecomment-625724677&data=02%7C01%7Cbart%40rutgers.edu%7C6abd33b3e0244c078c9908d7f330d89c%7Cb92d2b234d35447093ff69aca6632ffe%7C1%7C1%7C637245263431179967&sdata=HAECS9TTL8teOm8X%2B5Jb1b%2BbLrGugVfA4bE9c9jqHgE%3D&reserved=0, or unsubscribehttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAC3MRU72CGCLANUDJ7RME7DRQPFADANCNFSM4LJIZELA&data=02%7C01%7Cbart%40rutgers.edu%7C6abd33b3e0244c078c9908d7f330d89c%7Cb92d2b234d35447093ff69aca6632ffe%7C1%7C1%7C637245263431189966&sdata=OE7B50LWg7mdObKb4NR0hS4QdsirLhm6IXwtiNxUOVg%3D&reserved=0.

adammorrissirrommada commented 4 years ago

A lot of our experiments are already at or beyond threshold for frame drops, particularly in Shaun's rig, so I do think this will be a problem for us.

Would it work for your purpose if it was reverted to being in stimulus parent, but with diode code for drawing also called in beforeITIframe()?

bartkrekelberg commented 4 years ago

In principle yes (all I need is that the specified background color (which is different from the rest of the screen background) is drawn during the ITI.

I don't think adding this to the beforeItiFrame is easy to do, as you have to determine whether there is a diode flasher that needs to be drawn (which requires a loop through stimuli in the old diode code?) or some other tracking mechanism (which would add overhead not that different from the separate diodeFlasher stimulus?)


From: Adam Morris notifications@github.com Sent: Monday, May 11, 2020 11:19 To: klabhub/neurostim-ptb neurostim-ptb@noreply.github.com Cc: Bart Krekelberg bart@vision.rutgers.edu; State change state_change@noreply.github.com Subject: Re: [klabhub/neurostim-ptb] Diode (#149)

A lot of our experiments are already at or beyond threshold for frame drops, particularly in Shaun's rig, so I do think this will be a problem for us.

Would it work for your purpose if it was reverted to being in stimulus parent, but with diode code for drawing also called in beforeITIframe()?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHubhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fklabhub%2Fneurostim-ptb%2Fpull%2F149%23issuecomment-626581160&data=02%7C01%7Cbart%40rutgers.edu%7C68048c05947f463b26ee08d7f58c6ac1%7Cb92d2b234d35447093ff69aca6632ffe%7C1%7C0%7C637247855744046280&sdata=U%2BivtbHT5GGk2P1RgJ2JnR6o3rvGsB0PYWPrpBmsvnI%3D&reserved=0, or unsubscribehttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAC3MRUZTIV4VWM77RHV3M4LRQ67KBANCNFSM4LJIZELA&data=02%7C01%7Cbart%40rutgers.edu%7C68048c05947f463b26ee08d7f58c6ac1%7Cb92d2b234d35447093ff69aca6632ffe%7C1%7C0%7C637247855744046280&sdata=xkfHL%2FNK9gXX1Db0iZvHUb9oUUxnAWVodww34RKFwIg%3D&reserved=0.

adammorrissirrommada commented 4 years ago

I'm not following.... Couldn't any stimulus draw the diode square in iti (potentially with a separately specified colour) in its beforeITIframe()? Why is a loop over others needed? And if a loop is needed, isn't that overhead only between trials and not every frame?