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

Fix Issue 484 --- pspm_sf #483

Merged
merged 32 commits into from
Aug 11, 2023
Merged

Fix Issue 484 --- pspm_sf #483

merged 32 commits into from
Aug 11, 2023

Conversation

teddychao
Copy link
Contributor

@teddychao teddychao commented May 15, 2023

Fixes #484.

Introduction

Missing epochs often appear in the imported data. It is expected to let users know the imported data contain missing epochs since they impact the results of processing. Specifically for this issue, the source data contains missing epochs at the beginning. Since some processing requires the initial values for determine its parameters, such missing epochs finally lead to errors. At the same time, the missing epochs cannot be ignored in VBA processing, and therefore there are further issues inside VBA functions. This pull request aims at resolving such initial missing epochs that disallow following analysis.

Methods

  • When there are missing epochs, let users know missing epochs exist and the results will be affected.
    • Users can adjust the field options.missingthreshold to skip trials that contain missing epochs more than the threshold time.
  • Initial missing epochs should be filtered out when selecting initial values for determining some parameters.
  • Interpolation will be performed before processing VBA analysis.

Results

  • Overall

  • src/pspm_dcm_inv.m

    • DCM analysis requires the first three values of the imported data for determining priors. Now it uses non-*NaN- values for determine such parameters (Line 556 --- Line 559).
    • .missing_data as a field of model has been described in the help text.
    • .missing is not used thus removed from help text.
    • .missing_data does not require a default value because there may be no missing values.
  • src/pspm_options.m

    • Line 415: update the options for pspm_sf to allow users adjust the threshold.
      • The field will be transferred to sf_dcm when sf calls sf_dcm.
  • src/pspm_sf.m

    • Line 28--33: help text describes model.missing is used for specifying the missing epochs.
    • Line 183--192: model.missing is used to load missing epochs.
    • Line 219--238: Loading missing epochs.
    • Line 288--289: use pspm_time2index to load missing data, from time ('seconds') into timepoints.
  • src/pspm_sf_dcm.m

    • Line 115 --- Line 116
      • Now it uses only non-*NaN- values for determining X0 and nresp.
    • Line 154 --- Line 163
      • Now reminds users when there are missing epochs if it is the situation since the results may be dominated with NaN.
      • Now allows users to change the threshold for skipping trials that contain missing epochs if they wish.
    • Line 169
      • Now it uses pspm_interpolate to fill the NaNs before using VBA functions.

@teddychao teddychao self-assigned this May 15, 2023
@teddychao teddychao linked an issue May 15, 2023 that may be closed by this pull request
@teddychao teddychao marked this pull request as ready for review May 15, 2023 15:40
@teddychao teddychao requested a review from uzaygokay May 15, 2023 15:40
@teddychao teddychao added the Completed & Waiting for Review Completed and waiting for review label May 15, 2023
@teddychao teddychao added this to the v6.1 milestone May 15, 2023
@teddychao teddychao changed the title Fix sf issue Fix Issue 484 --- pspm_sf May 15, 2023
@teddychao teddychao changed the title Fix Issue 484 --- pspm_sf Fix Issue 484 --- pspm_sf May 15, 2023
@teddychao teddychao added the Non-Conceptional The pull request does not include conceptional updates label May 15, 2023
@teddychao teddychao marked this pull request as draft May 15, 2023 16:25
@teddychao teddychao removed Completed & Waiting for Review Completed and waiting for review Non-Conceptional The pull request does not include conceptional updates labels May 15, 2023
By changing only internal PsPM code
src/pspm_sf_dcm.m Outdated Show resolved Hide resolved
@teddychao teddychao marked this pull request as ready for review June 5, 2023 15:42
@teddychao teddychao added the Completed & Waiting for Review Completed and waiting for review label Jun 5, 2023
src/pspm_sf_dcm.m Outdated Show resolved Hide resolved
@teddychao
Copy link
Contributor Author

Hi @dominikbach @uzaygokay many thanks for approving this. I will send this updated package to Carlos for him to test. I will merge it when he feels comfortable with the updated code. Thank you

src/pspm_sf_dcm.m Outdated Show resolved Hide resolved
src/pspm_sf_dcm.m Outdated Show resolved Hide resolved
src/pspm_sf_dcm.m Outdated Show resolved Hide resolved
src/pspm_sf_dcm.m Outdated Show resolved Hide resolved
@dominikbach
Copy link
Contributor

I realise now there is a more fundamental problem.

pspm_sf takes one or multiple data files as data inputs, and an epoch definition file as event input. It will then pass the data to pspm_sf_dcm for each identified epoch.

This means that the missing epoch definition needs to be retrieved in pspm_sf (if method is "dcm"), and a missing index needs to be passed on to pspm_sf_dcm.

Copy link
Contributor

@dominikbach dominikbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions and comments.

src/pspm_sf_dcm.m Outdated Show resolved Hide resolved
src/pspm_sf_dcm.m Outdated Show resolved Hide resolved
src/pspm_sf.m Show resolved Hide resolved
@dominikbach dominikbach added Discussion Further information is requested and removed Completed & Waiting for Review Completed and waiting for review labels Jun 30, 2023
@teddychao teddychao added Completed & Waiting for Review Completed and waiting for review and removed Discussion Further information is requested labels Jul 24, 2023
@teddychao
Copy link
Contributor Author

I realise now there is a more fundamental problem.

pspm_sf takes one or multiple data files as data inputs, and an epoch definition file as event input. It will then pass the data to pspm_sf_dcm for each identified epoch.

This means that the missing epoch definition needs to be retrieved in pspm_sf (if method is "dcm"), and a missing index needs to be passed on to pspm_sf_dcm.

I think this issue has been resolved through the recent pull request #502 . I have merged #502 into this pull request.

src/pspm_cfg/pspm_cfg_sf.m Outdated Show resolved Hide resolved
@dominikbach dominikbach merged commit 3ce8c7c into develop Aug 11, 2023
1 check passed
@dominikbach dominikbach deleted the sf_dcm_nan_issue branch August 11, 2023 08:48
@teddychao teddychao added Solved and removed Completed & Waiting for Review Completed and waiting for review 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 this pull request may close these issues.

Missing epoch issue of pspm_sf
3 participants