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

Change of behavior of om_gain: it used to apply weights #353

Closed
ftadel opened this issue Mar 13, 2019 · 3 comments
Closed

Change of behavior of om_gain: it used to apply weights #353

ftadel opened this issue Mar 13, 2019 · 3 comments

Comments

@ftadel
Copy link

ftadel commented Mar 13, 2019

The function om_gain used to return only one row per sensor, no matter how many integration points are used for these points. Now it returns one row per integration point.

In the example below, I have processed the same CTF example with 9 magnetometers (4 integration points) and 291 gradiometers (8 integration points) from Brainstorm with two versions of OpenMEEG:
https://www.dropbox.com/s/q1e9dfvalmz5hwo/openmeeg-2.1.zip?dl=0
https://www.dropbox.com/s/7m4y85ojxluk2a0/openmeeg-2.4.zip?dl=0

OpenMEEG 2.1: openmeeg_gain_meg.mat = [300 x 3009]
OpenMEEG 2.4: openmeeg_gain_meg.mat = [2364 x 3009]

This causes the current Brainstorm wrapper not to work anymore, as there is nothing to apply the weights after the call to om_gain:
https://github.com/brainstorm-tools/brainstorm3/blob/master/toolbox/forward/bst_openmeeg.m#L458

And therefore users complaining:
https://neuroimage.usc.edu/forums/t/issue-forward-modelling-with-openmeeg-bem/10266/
https://neuroimage.usc.edu/forums/t/error-in-openmeeg-in-meg-data/8421

Is this related to this other issue?
#347

Did this become an optional parameter controlled with a command line flag?
Is it something that you will fix, or something that should be handled in the Brainstorm side instead? If so, what should we do: simply sum the rows multiplied by the weights?

@agramfort
Copy link
Contributor

thanks @ftadel for the bug report... let us look into it ASAP.

@ftadel
Copy link
Author

ftadel commented Mar 14, 2019

I confirm that replacing the sensor name from a simple index "1" to a text label "MEG001" fixes the issue:
brainstorm-tools/brainstorm3@7e463aa#diff-9e590703ce3a61a47c365d821b1164e6

@agramfort
Copy link
Contributor

thanks

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