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 338 - pspm_glm #339

Merged
merged 6 commits into from
Jan 7, 2022
Merged

Fix issue 338 - pspm_glm #339

merged 6 commits into from
Jan 7, 2022

Conversation

teddychao
Copy link
Contributor

@teddychao teddychao commented Dec 13, 2021

Fixes #338.

Changes proposed in this pull request:

  • Add a warning when input data was filtered out more than 10% due to missing epoch detection.
  • Specify the reason of missing epochs in the warning after checking sampling rates.

@teddychao teddychao changed the title Add warning reporting when missing epochs filtered more than 10% of data Fix issue 338 - pspm_glm Dec 13, 2021
@teddychao teddychao marked this pull request as draft December 13, 2021 19:24
@teddychao teddychao self-assigned this Dec 13, 2021
@teddychao teddychao marked this pull request as ready for review December 14, 2021 18:46
@teddychao teddychao added Improvement A minor improvement to the project Waiting for Approval labels Dec 14, 2021
@teddychao teddychao added this to the v5.1.2 milestone Dec 18, 2021
src/pspm_glm.m Outdated
else
warning('ID:invalid_input', ...
['More than 10% of input data was filtered out due to missing epochs, ',...
'which is highly likely caused by downsampling. Results may be inaccurate.']);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say "is possibly caused by downsampling" - the data might just as well have been missing in the first place, before downsampling.

@dominikbach dominikbach merged commit 30abe36 into develop Jan 7, 2022
@teddychao teddychao modified the milestones: v5.1.2, v6.0.0 Jan 24, 2022
@teddychao teddychao mentioned this pull request Jul 12, 2022
@dominikbach dominikbach deleted the Fix-issue-338 branch December 13, 2022 16:35
@m-guggenmos
Copy link

Just to make sure, is this line in the fix correct?

perc_missing = 1 - sum(glm.M)/length(glm.M);

I'm getting a warning related to this variable a lot and intuitively it makes more sense without the 1 -? I'm not familiar with the codebase though.

@teddychao teddychao restored the Fix-issue-338 branch February 8, 2023 18:14
@teddychao
Copy link
Contributor Author

Just to make sure, is this line in the fix correct?

perc_missing = 1 - sum(glm.M)/length(glm.M);

I'm getting a warning related to this variable a lot and intuitively it makes more sense without the 1 -? I'm not familiar with the codebase though.

Hi @m-guggenmos,
Thank you for propose the issue to us. Yes it is a mistake. The positive values in M and glm.M denote missing values, thus the perc.missing should not contain a 1- there.
We apologise for any inconenience the mistake brought to you. We will include this fix in our next release that will be available very soon.
Best wishes
Teddy

@m-guggenmos
Copy link

Great, thanks for the prompt response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement A minor improvement to the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pspm_glm produce no results when facing many short missing epochs
3 participants