kymata-atlas / kymata-core

Core Kymata codebase, including statistical analysis and plotting tools
https://kymata.org
MIT License
5 stars 0 forks source link

Apply stimulus shift and latency correction inside gridsearch, not in the preprocessing steps #290

Closed caiw closed 2 months ago

caiw commented 2 months ago

The problem

On running gridsearch on visual functions on the Russian dataset, I found that it was working but the latency was obviously wrong:

MicrosoftTeams-image

On searching through the code, @neukym and I realised the mistake: the data preprocessing was applying latency correction for audio-delivered stimuli, but this wasn't actually being used, and the gridsearch was applying latency-stretch correction for audio-delivered stimuli; and neither were implemented for visual-delivered stimuli.

After discussion with @neukym, we decided that because the shift/stretch correction is (potentially) different for each stimulus-delivery modality, it makes sense to do this online, tailored to the specific function being run, during the gridsearch, instead of separately preprocessing the trialwise data for each modality.

This PR

For the reviewer

@ollieparish I'm asking you for review as you wrote most of this code! A few questions and notes we had while reading/writing the code.

@neukym - request for testing to confirm syntax breaks and results matches the previous results.

Also covered in this PR (via @neukym)

neukym commented 2 months ago

last commit addressed most of #291

neukym commented 2 months ago

I've found a possible major problem with trying to fix things this way for the visual stream - I have found at least one example in dataset 4 where the visual frames pause for a whole two seconds, and then carry on. (participant 1, second presentation).

This means most of that repetition is out by a whole 2000ms - i.e. unusable if we use this method. The other three reps I looked at were very stable (exactly the same, continuous drift) - but I don't know how common this 'visual frames pause' issue is. Perhaps we should be checking for this and then rejecting any reps that have this issue?

(the whole reason that I put in triggers every second was to guard against this possibility – but I didn't check it really was necessary)

Depending how many 'pauses' are present, we may have to use the events as an override of some sort. New issue: #295

@ollieparish can you still review this PR, but don't merge.

neukym commented 2 months ago

@neukym confirmed it works for English TVL (I had to redo the trialwise data for it to work, found some minor bugs, now fixed.): visual_stimuli_refactor_test_dataset4_TVL_family_after_270_rescaled

neukym commented 2 months ago

@neukym confirmed it works for Russian TVL: visual_stimuli_refactor_test_dataset4_TVL_family_after_270_rescaled_russian

neukym commented 2 months ago

@neukym confirmed it works for Russian CIECAM02, and fixes issue. visual_stimuli_refactor_test_dataset3_ciecam02