search

bachlab / PsPM

A matlab suite for Psycho-Physiological Modelling
GNU General Public License v3.0
45 stars 11 forks source link

pspm_sf has issues when processing data whose unit is defined by marker #503

Closed teddychao closed 1 year ago

teddychao commented 1 year ago

Summary

pspm_sf is reported to have issues when processing data whose time unit is defined as marker. Further investigation is required to validate whether the win (line 281 in pspm_sf) is calculated correctly.

dominikbach commented 1 year ago

I guess line 282 in pspm_sf should be replaced by

win = round(events(epochs{iFile}(iEpoch, :)) * sr(datatype(k)));

teddychao commented 1 year ago

I guess line 282 in pspm_sf should be replaced by

win = round(events(epochs{iFile}(iEpoch, :)) * sr(datatype(k)));

Yes, this had been implemented before your comment, but the error (which may be caused by inappropriate parameter settings) seems to be still there.

I observed the data itself, and I am unsure if some settings are correct.

  1. Based on the provided data (from our user), it seems like in line 282 the variable events (read from data.data) is between 1.1910 and 2.5228, and its unit is uS and the sampling frequency / rate is 16. I understand the standard unit of sr is Hz. In this case, I am not sure if the win here calculated as uS multiplied by Hz is correct. For the line 277, it makes sense to me the unit of epochs is second and the unit of sr is Hz so the result is time point, but the line 282 is very strange to me. I am not sure if the line 282 is based on the assumption that the unit of events must be second instead of uS?
  2. I have a question about line 227 at pspm_sf_mp, the variable asf. It was calculated at line 191 only if a(k,1)<=0. However this is not always satisfied thus sometimes asf will not be generated? I am unsure if line 227 should be executed only if a(k,1)<=0 is satisfied?
dominikbach commented 1 year ago
  1. There seems to be a bug in lines 262-269. If no marker channel number is specified (line 262) then the marker channel is read (line 263) but the data are not used, and instead line 269 will use the previously loaded SCR data.

I think the whole if-else statement (line 245/262/270) can be simplified as it simply duplicates code depending on whether options.marker_chan_num is specified or not. If "options.marker_chan_num" is not specified (line 245), then this variable can be simply set to "marker", and the else statement is not further needed.

  1. Can you specify which version of the code you are referring to? In the develop branch, line 227 does not operate on asf. In this version, asf is calculated in line 184 but only if a(k,1) > 0 (else-statement). a(k,1) <= 0 is the stopping criterion, and the assumption is that asf is already defined. It might happen in edge cases that the stopping criterion is fulfilled on the first pass; so it would make sense to initialise it before the while-loop.
teddychao commented 1 year ago
  1. There seems to be a bug in lines 262-269. If no marker channel number is specified (line 262) then the marker channel is read (line 263) but the data are not used, and instead line 269 will use the previously loaded SCR data.

I think the whole if-else statement (line 245/262/270) can be simplified as it simply duplicates code depending on whether options.marker_chan_num is specified or not. If "options.marker_chan_num" is not specified (line 245), then this variable can be simply set to "marker", and the else statement is not further needed.

  1. Can you specify which version of the code you are referring to? In the develop branch, line 227 does not operate on asf. In this version, asf is calculated in line 184 but only if a(k,1) > 0 (else-statement). a(k,1) <= 0 is the stopping criterion, and the assumption is that asf is already defined. It might happen in edge cases that the stopping criterion is fulfilled on the first pass; so it would make sense to initialise it before the while-loop.

Many thanks. The first comment has resolved the issue now.

  1. I have also updated the code. I feel the logic is now a little different but clearer to me. Previously, the previous version used the marker loading as a fallback from marker_num_chan if it could not load successfully, but now it will only report an error and return simply. However I feel this is clearer and simpler to me.. By updating the event reading, the issue has been updated now.
  2. Apologies, I was referring to the branch #502 when I was talking about these variables. However what you commented had answered my question. I have now set up the initial values for these variables. This error is no longer appearing in the test data from the user now.