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

add the possibility to compute the MEG (forward FEM) with a block of sensors #367

Conversation

tmedani
Copy link
Member

@tmedani tmedani commented Dec 1, 2020

Hello all,
@ftadel, to complete the previous PR, this new PR adds the option to compute the MEG FEM forward with a group of sensors, this will reduce the size of the transfer matrix, therefore the memory required for the computation, and avoid the computation crashes.
I have fixed the max number of sensors to 250 for each iteration.

The new duneuro panel includes a check box for this option
image

I have also validated the output results with this option.

@ftadel I have attached some questions to the scripts, let me know if there are any changes that I can do.

Thanks :),
Takfarinas

@@ -523,7 +523,47 @@
disp(['DUNEURO> System call: ' callStr]);
tic;
% Call DUNEuro
[status,cmdout] = system(callStr);
cfg.SizeOfBlockOfSensor = 250; % we may ask the user to change this value from the panel? Q for Francois
Copy link
Member

Choose a reason for hiding this comment

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

250?
For a 256-electrode EEG channel, it means that it would run once with 250 and once with 6 sensors?
For an Elekta MEG: one execution with 250 and one with 56?
This sounds like a weird logic.

Have you tried to quantify the increase in computation time vs. the decrease in memory usage?
For example: when you divide the number of sensors by 2, by 4, by 8.
If you need to split this list, I guess the most optimal way to balance the computation time and the memory usage would be to split in equal blocks, isn't it?

I am not in favor of adding useless parameters that no one will ever modify. Maybe one check box that allows to divide in 2 or 4 could be useful for re-running when it crashes because of memory errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

250?
For a 256-electrode EEG channel, it means that it would run once with 250 and once with 6 sensors?
For an Elekta MEG: one execution with 250 and one with 56?
This sounds like a weird logic.

as discussed, this is not for EEG but for MEG sensors where four integrations points are used for magnetometers and 8 for gradiometers, so the total number of points for computation is around 8x250 ~= 2000

Have you tried to quantify the increase in computation time vs. the decrease in memory usage?
For example: when you divide the number of sensors by 2, by 4, by 8.

Yes, in some basic case, less block faster is the solution, with more block the process of reading/writing is multiplied.

Maybe one check box that allows to divide in 2 or 4 could be useful for re-running when it crashes because of memory errors?

ok, this is another useful option, then I can modify the code to this option

% solution : concatenate and saveback the transfer matrix?, or
% disable this option in this case, ask Francois how to do it from the GUI?
% when the option "use meg block sensor" is set to 1, disable the save of the
% transfer matrix.? Q for Francois
Copy link
Member

Choose a reason for hiding this comment

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

In panel_duneuro/UpdatePanel, add something like:

if ~isempty(jCheckMegPerBlockOfSensor)
    jCheckMegPerBlockOfSensor.setEnabled(~jCheckSaveTransfer.isSelected());
    if jCheckSaveTransfer.isSelected()
        jCheckMegPerBlockOfSensor.setSelected(0);
    end
end

You could also rename jCheckMegPerBlockOfSensor into jCheckMegBlock.
These super long names are painful to read.

And remove this comment :)

@@ -254,6 +254,13 @@
if (OPTIONS.BstSaveTransfer)
jCheckSaveTransfer.setSelected(1);
end
% disable the save transfer option if the option per block is
Copy link
Member

Choose a reason for hiding this comment

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

Delete this section, move it to UpdatePanel()

@ftadel
Copy link
Member

ftadel commented Apr 5, 2021

Can you please fix the conflicts?

@ftadel
Copy link
Member

ftadel commented Apr 5, 2021

Hmmm... I thought you had just posted something in the PR, Github sent me a notification...
But looking at it now, it looks like an old branch. Not sure what happened, sorry.

If this is outdated, please close the PR - thanks

@tmedani
Copy link
Member Author

tmedani commented Apr 5, 2021

I have a newest version that I will clean and push asap

@tmedani tmedani closed this Apr 5, 2021
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.

2 participants