fix issue 499

[dominikbach]

Fixes # 499.

Changes proposed in this pull request:

• added new input mode for pspm_get_timing: "missing"
• uses recursive call to pspm_get_timing("epochs", ...), then sorts & merges missing epochs
• respective code blocks in pspm_sf and pspm_dcm are removed
• fixes bug in missing epoch merging sorting
• adds additional check on epochs (offset values must be larger than onset values)
• fixes various bugs in epoch & missing epoch handling in pspm_sf

[dominikbach]

Requires testing.

[dominikbach]

Tested various use-cases on dummy data. Please review.

[teddychao]

Tested user's data for SF analysis. With or without missing data, both works ok.

[teddychao]

Hi @dominikbach, here are some comments/questions for the updates of SF and some other related functions.

• src/pspm_cfg/pspm_cfg_sf.m and src/pspm_cfg/pspm_cfg_run_sf.m
  • This is my update. I modified the UI since the old UI seems to crash when trying to load missing data file from the UI. Now it is working.
• src/pspm_dcm.m, src/pspm_glm.m
  • The update seems to be applying the updated get_timing to pspm_dcm and pspm_glm, which makes sense to me.
• src/pspm_get_timing.m
  • The update seems to include missing as an option for get_timing.
  • I am unsure about the reason for line 396 to 399. I understand this as a checking step but I did not see this before when I was testing it.
• src/pspm_interp1.m
  • I am unsure about the reason of doing this update. May I ask if my old code leads to an issue when testing some data?
• src/pspm_sf.m
  • Line 79--85: I added the initialisation step for varargout, which I explained in Varargout needs to be assigned with default values at initialisation #516 .
  • Line 213: Maybe I misremembered. Our previous discussion is, if the user specified not to overwrite, then there won't be a warning. I am unsure if I am wrong for this point.
  • Line 262: I put events{iFile} = data{1}.data(:); outside the loop because I observed it loaded from data rather than data, and data was not contained in the for loop. If you check the original version of this line of code: https://github.com/bachlab/PsPM/blob/21f4eedb270ed59293cd43d38fbe76e83ce53c9c/src/pspm_sf.m, at Line 237, it writes events = data{1}.data. I admit it should be my responsibility to correct this, but I did not see the error when I tested, and that's why I was not aware. Perhaps we can discuss how I should improve my code testing procedure...
  • Line 277: this is my fault. I managed to correct events → events{iFile} at line 265, but I ignored this line.
  • Line 290--293: this seems to be adding a new restriction / checking step for data. I did not meet this issue when I use the user's data. I should test it with public data for SF next time.
  • Line 299--303 (original version): I am unsure about the reason why they are removed..
  • Line 306: I understood it as the window is applicable to the original data, and users have originally specified this in the missing data, since missing data file will only highlight whether there is missing values... My understanding seems to be different from what is expected according to the update.
  • Line 310--320: seems to be updates for data if inv_flag is positive.

[dominikbach]

• The update seems to include missing as an option for get_timing.
• I am unsure about the reason for line 396 to 399. I understand this as a checking step but I did not see this before when I was testing it.

This is an new and additional input check. If onsets are larger than offsets, then this is nonsensical - and it might lead to problems or unexpected results down the line.

• src/pspm_interp1.m

  • I am unsure about the reason of doing this update. May I ask if my old code leads to an issue when testing some data?

• src/pspm_sf.m

In the old version, lines 72-74 did not work. In line 72-73, you create an index (rather than a logical index). In line 74, you try to create something like "a complement of a logical index", but applied to the integer index this leads to nonsensical results (and in fact, to negative index values which causes an error in line 76).

• Line 213: Maybe I misremembered. Our previous discussion is, if the user specified not to overwrite, then there won't be a warning. I am unsure if I am wrong for this point.

I did not change anything here. If your recollection of the discussion is that there should not be a warning then why did you add a warning? In any case, this is a general issue that should be consistent across PsPM and should not be included here. Please revert.

• Line 290--293: this seems to be adding a new restriction / checking step for data. I did not meet this issue when I use the user's data. I should test it with public data for SF next time.

I tested it already. If there are fewer than 3 data points that pspm_sf_dcm cannot compute the initial value priors.

• Line 299--303 (original version): I am unsure about the reason why they are removed..

This is just moved outside the loop (lines 237-240 of the new version). Because missing data are specified per file (not per epoch), the index is computed per file and this does not have to repeated for every epoch.

• Line 306: I understood it as the window is applicable to the original data, and users have originally specified this in the missing data, since missing data file will only highlight whether there is missing values... My understanding seems to be different from what is expected according to the update.

Missing index is specified per file. When we select part of the data as epoch for processing, we also need to select the respective missing index. To illustrate, when the user says "seconds 10-15 of my data file is missing" and selects seconds 20-60 for epoch 1, then in your previous version seconds 10-15 of the epoch would have been classified as missing, corresponding seconds 30-45 of the original data, which is clearly wrong.

• Line 310--320: seems to be updates for data if inv_flag is positive.

We need to specify some output even if the inversion is not done.

[teddychao]

• The update seems to include missing as an option for get_timing.
• I am unsure about the reason for line 396 to 399. I understand this as a checking step but I did not see this before when I was testing it.

This is an new and additional input check. If onsets are larger than offsets, then this is nonsensical - and it might lead to problems or unexpected results down the line.

I understand now. May I confirm if this is caused by the bug from test data, e.g. from https://zenodo.org/record/3900169? I am trying to confirm I am using the correct data to test.

• Line 213: Maybe I misremembered. Our previous discussion is, if the user specified not to overwrite, then there won't be a warning. I am unsure if I am wrong for this point.

I did not change anything here. If your recollection of the discussion is that there should not be a warning then why did you add a warning? In any case, this is a general issue that should be consistent across PsPM and should not be included here. Please revert.

My brain was not working at that time, sorry for adding this unnecessary code. I just came up with that I added the code because the script return without showing anything when I set it to be "not to overwrite". I have inverted it now.

• Line 290--293: this seems to be adding a new restriction / checking step for data. I did not meet this issue when I use the user's data. I should test it with public data for SF next time.

I tested it already. If there are fewer than 3 data points that pspm_sf_dcm cannot compute the initial value priors.

I see. May I just confirm with you that the data you used to test is from here: https://zenodo.org/record/3900169?

• Line 299--303 (original version): I am unsure about the reason why they are removed..

This is just moved outside the loop (lines 237-240 of the new version). Because missing data are specified per file (not per epoch), the index is computed per file and this does not have to repeated for every epoch.

• Line 306: I understood it as the window is applicable to the original data, and users have originally specified this in the missing data, since missing data file will only highlight whether there is missing values... My understanding seems to be different from what is expected according to the update.

Missing index is specified per file. When we select part of the data as epoch for processing, we also need to select the respective missing index. To illustrate, when the user says "seconds 10-15 of my data file is missing" and selects seconds 20-60 for epoch 1, then in your previous version seconds 10-15 of the epoch would have been classified as missing, corresponding seconds 30-45 of the original data, which is clearly wrong.

I have now understood the logic of missing index. Previously, I received Carlos' data where the initial several values are missing, and I tested SF with his data where I did not consider file specification for missing. Thanks for pointing out this.

• Line 310--320: seems to be updates for data if inv_flag is positive.

We need to specify some output even if the inversion is not done.

I understand and agree with the newly implemented code. I think the indention is a little strange but should not affect the performance. I think the develop branch is in line with the original version of SF at https://github.com/bachlab/PsPM/blob/21f4eedb270ed59293cd43d38fbe76e83ce53c9c/src/pspm_sf.m, and it has not been changed for a while, so this update is necessary for it..

[dominikbach]

• The update seems to include missing as an option for get_timing.
• I am unsure about the reason for line 396 to 399. I understand this as a checking step but I did not see this before when I was testing it.

This is an new and additional input check. If onsets are larger than offsets, then this is nonsensical - and it might lead to problems or unexpected results down the line.

I understand now. May I confirm if this is caused by the bug from test data, e.g. from https://zenodo.org/record/3900169? I am trying to confirm I am using the correct data to test.

There is no faulty missing index in the test data; I have created this edge case manually.

• Line 290--293: this seems to be adding a new restriction / checking step for data. I did not meet this issue when I use the user's data. I should test it with public data for SF next time.

I tested it already. If there are fewer than 3 data points that pspm_sf_dcm cannot compute the initial value priors.

I see. May I just confirm with you that the data you used to test is from here: https://zenodo.org/record/3900169?

Again, such missing indices are not contained in any test data but you can easily create them.

[teddychao]

• I am unsure about the reason of doing this update. May I ask if my old code leads to an issue when testing some data?
• src/pspm_sf.m

In the old version, lines 72-74 did not work. In line 72-73, you create an index (rather than a logical index). In line 74, you try to create something like "a complement of a logical index", but applied to the integer index this leads to nonsensical results (and in fact, to negative index values which causes an error in line 76).

You are right my original version does not seem to be correct. Since it gives incorrect results it should be fixed.
Just to say in Matlab it is recognised to use integer to & or | between integers and logical numbers, and integers are considered as a logical 1. However, this does not seem to be intuitive. I failed to test and detect this mistake.

[teddychao]

Missing data can be correctly transferred to the tune 304 in `SF`, after testing.

[teddychao]

@dominikbach I added minor changes to your latest version

1. Correct the GUI's field missing assigning, which should be in model than options.
2. I indent your code between 309 and 321.

If you are happy with these changes, I can merge this pull request to the develop branch. Thanks