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

GLM: Fix all_stats() segfault #2294

Merged
merged 2 commits into from
Mar 4, 2021
Merged

GLM: Fix all_stats() segfault #2294

merged 2 commits into from
Mar 4, 2021

Conversation

Lestropie
Copy link
Member

As reported in #2289.

Fixes segmentation fault in case where:

  1. Either:
    1. Input data contain non-finite values (regardless of whether or not those data are or are not within the statistical inference processing mask);
    2. Element-wise design matrix columns are added via -column option;
  2. At least one F-test is performed.

Fairly obvious blunder even without explaining the surrounding code.

In instances where the input data contain non-finite values (regardless of whether or not such data are excluded from statistical inference via a mask), and at least one F-test is performed, writing the absolute and standardised effect size as NaN (as such cannot be meaningfully calculated for an F-test) was incorrectly indexed, leading to a segmentation fault.
@Lestropie Lestropie added the bug label Mar 3, 2021
@Lestropie Lestropie added this to the 3.0.3 hotfix milestone Mar 3, 2021
@Lestropie Lestropie requested a review from a team March 3, 2021 02:37
@Lestropie Lestropie self-assigned this Mar 3, 2021
Copy link
Member

@jdtournier jdtournier left a comment

Choose a reason for hiding this comment

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

I'm not sure I fully appreciate what each row of your local_abs_effect_size and local_std_effect_size is supposed to contain, but I'll take your word for it that it's appropriate to set the first row to NaN rather than the second... 😁

@Lestropie
Copy link
Member Author

Lestropie commented Mar 4, 2021

I'm not sure I fully appreciate what each row of your local_abs_effect_size and local_std_effect_size is supposed to contain ...

In cases where the GLM can't be fit to the exhaustive set of data in a single matrix operation, the relevant code loops over elements, each thread processes one element at a time. So any GLM derivative that would normally have one row per element becomes a row vector in that context. But they're treated as matrices with one row rather than explicit vectors so that the same function can be used as that used when all data are solved at once.

@Lestropie Lestropie merged commit 430465d into master Mar 4, 2021
@Lestropie Lestropie deleted the glm_allstats_fix branch March 4, 2021 02:23
@Lestropie Lestropie modified the milestones: 3.0.4 hotfix, 3.0.3 hotfix May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants