klabhub / neurostim

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

fix eyelink.valid handling when .loc_useRawData == true #228

Closed dshimaoka closed 1 month ago

dshimaoka commented 1 month ago

When eyelink plugin is operated with .loc_useRawData = false, .valid stays always false even during blinking. This causes unintended behaviour of eyeMovement.isInWindow, always returning false.

The unintended behavior of .valid stems from its definition: o.valid = any(sample.gx(eyeNr+1)~=o.el.MISSING_DATA); When operated with .loc_useRawData = true, gx always returns el.MISSING_DATA (=-32768). Thus .valid stays false.

Instead, px returns sensible values during successful eye tracking and returns el.MISSING_DATA (=-32768) only during unsuccessful eye tracking such us during blinking.

Hence, a simple fix to this problem is to define .valid by px as:

if o.loc_useRawData
o.valid  = any(sample.px(eyeNr+1)~=o.el.MISSING_DATA);
end
adammorrissirrommada commented 1 month ago

This looks right to me, but I will leave it for @cnuahs to confirm.

adammorrissirrommada commented 1 month ago

@dshimaoka, thanks for this pull request. for future PRs, please add an assignee (Shaun, Bart, or me) and at least one reviewer (can be the same as assignee if you like) so that it is on someone's desk as a to-do.

dshimaoka commented 1 month ago

@dshimaoka, thanks for this pull request. for future PRs, please add an assignee (Shaun, Bart, or me) and at least one reviewer (can be the same as assignee if you like) so that it is on someone's desk as a to-do.

To @adammorrissirrommada I suspect I am not authorized to add an assignee (https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/requesting-a-pull-request-review)

adammorrissirrommada commented 1 month ago

@dshimaoka,

Ah, I see. in that case, just tag one of us in a comment to ask us to assign it. thanks.

adammorrissirrommada commented 1 month ago

@bartkrekelberg we're OK with this change, but giving you final say over the merge because it affects a fundamental plugin.