Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
teddychao opened this issue Jul 17, 2023 · 4 comments · Fixed by #502
Closed

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

teddychao opened this issue Jul 17, 2023 · 4 comments · Fixed by #502
Assignees
Milestone

Comments

@teddychao
Copy link
Contributor

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.

@teddychao teddychao self-assigned this Jul 17, 2023
@teddychao teddychao added the Bug Something isn't working label Jul 17, 2023
@teddychao teddychao added this to the v6.1 milestone Jul 17, 2023
@teddychao teddychao linked a pull request Jul 17, 2023 that will close this issue
@dominikbach
Copy link
Contributor

I guess line 282 in pspm_sf should be replaced by

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

@teddychao
Copy link
Contributor Author

teddychao commented Jul 23, 2023

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
Copy link
Contributor

  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
Copy link
Contributor Author

teddychao commented Jul 24, 2023

  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 Fix issue 503 --- pspm_sf relevant functions #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.

@teddychao teddychao added Solved and removed Bug Something isn't working labels Aug 20, 2023
@teddychao teddychao removed the Solved label Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants