search

translationalneuromodeling / tapas

TAPAS - Translational Algorithms for Psychiatry-Advancing Science
https://translationalneuromodeling.github.io/tapas/
GNU General Public License v3.0
217 stars 90 forks source link

[PhysIO] rpulset_padded : out of memory #166

Closed benoitberanger closed 2 years ago

benoitberanger commented 2 years ago

Hello,

Version v5.1.2

commit 7101195d4104c3a4a14e28176fff9d72b4fb3e4d (HEAD, tag: v5.1.2)
Author: Inês Pereira <pereira@biomed.ee.ethz.ch>
Date:   Thu Sep 2 14:18:05 2021 +0000

    Update CHANGELOG.md

Logs

------------------------------------------------------------------------
09-Dec-2021 19:02:01 - Running job #1
------------------------------------------------------------------------
09-Dec-2021 19:02:01 - Running 'TAPAS PhysIO Toolbox'
Warning: Ignoring given sampling intervals, using dt = 2.5 ms (tics) instead 
> In tapas_physio_log (line 58)
  In tapas_physio_read_physlogfiles_siemens_tics (line 85)
  In tapas_physio_read_physlogfiles (line 92)
  In tapas_physio_main_create_regressors (line 130)
  In tapas_physio_cfg_matlabbatch>run_physio (line 1661)
  In cfg_run_cm (line 29)
  In cfg_util>local_runcj (line 1717)
  In cfg_util (line 972)
  In cfg_ui>MenuFileRun_Callback (line 710)
  In gui_mainfcn (line 95)
  In cfg_ui (line 53) 
Warning: Invalid trend type '3'.. assuming 'linear'. 
> In detrend (line 52)
  In tapas_physio_filter_respiratory (line 68)
  In tapas_physio_main_create_regressors (line 218)
  In tapas_physio_cfg_matlabbatch>run_physio (line 1661)
  In cfg_run_cm (line 29)
  In cfg_util>local_runcj (line 1717)
  In cfg_util (line 972)
  In cfg_ui>MenuFileRun_Callback (line 710)
  In gui_mainfcn (line 95)
  In cfg_ui (line 53) 
09-Dec-2021 19:02:05 - Failed  'TAPAS PhysIO Toolbox'
Out of memory. Type HELP MEMORY for your options.
In file "/matlab/tapas/PhysIO/code/preproc/tapas_physio_filter_respiratory.m" (???), function "tapas_physio_filter_respiratory" at line 133.
In file "/matlab/tapas/PhysIO/code/tapas_physio_main_create_regressors.m" (???), function "tapas_physio_main_create_regressors" at line 218.
In file "/matlab/tapas/PhysIO/code/tapas_physio_cfg_matlabbatch.m" (???), function "run_physio" at line 1661.

Debug

Function name : tapas_physio_filter_respiratory.m Line : 133 Content : rpulset_padded(1:n_pad) = padding_window(1:n_pad) .* rpulset_padded(1:n_pad);

Cause

The dimensions do not agree to perform element wise product : Currently, I have :

rpulset_padded(1:n_pad) = padding_window(1:n_pad) .* rpulset_padded(1:n_pad);
[N x 1] = [1 x N] .* [N x 1]

So, since it's an element wise multiplicatication of a column vector and a row vector, it tries to create a new [N x N] matrix. From reading the code, there is no intention to create a [N x N] matrix, the dimension problem comes from above.

From the top of the function, rpulset is column vector, and this do not change. Then rpulset_padded is also a column vector. Finally, padding_window is row vector. And the element wise multiplication occures, and the crash occures.

Thank you for your help, Benoît

mrikasper commented 2 years ago

Dear Benoît,

it's funny you found this behavior right now, because we (@stebo85, @gllmflndn and me) encountered the same issue last week during BrainHack Global 2021 when compiling SPM12 with PhysIO.

Let me know whether this fixes the problem!

All the best, Lars

PS: On the upside, the discussion around this issues led to a standalone SPM12+PhysIO version that doesn't need a Matlab license and is now available on NeuroDesk @NeuroDesk

benoitberanger commented 2 years ago

Hi Lars,

Indeed, I have unexpected sub-path from SPM12 and FieldTrip in the Matlab path. I must have used savepath() at some point without checking if I had undesired path in the current list. I just removed the sub-path, and the error vanished.

After checking a bit deeper, here is my findings : I stopped the code at line 131 padding_window = window(@blackmanharris, 2 * n_pad + 1);

131 padding_window = window(@blackmanharris, 2 * n_pad + 1);
K>> which window -all
/matlab/R2017b/toolbox/signal/signal/window.m
/matlab/R2017b/toolbox/signal/signal/+fdesign/@abstracttype/window.m  % Shadowed fdesign.lowpass method
/matlab/R2017b/toolbox/signal/signal/+fspecs/@abstractspec/window.m   % Shadowed fspecs.lpmin method
K>> padding_window = window(@blackmanharris, 2 * n_pad + 1); size(padding_window)
ans =
      160003           1
K>> ft_defaults()
K>> which window -all
/matlab/fieldtrip/external/signal/window.m
/matlab/R2017b/toolbox/signal/signal/+fdesign/@abstracttype/window.m  % Shadowed fdesign.lowpass method
/matlab/R2017b/toolbox/signal/signal/+fspecs/@abstractspec/window.m   % Shadowed fspecs.lpmin method
/matlab/R2017b/toolbox/signal/signal/window.m                         % Shadowed
K>> padding_window = window(@blackmanharris, 2 * n_pad + 1); size(padding_window)
ans =
           1      160003

It means that FieldTrip has also it's own version of window function, shadowing the version provided by Matlab's Signal toolbox.

I agree this is not a "bug" since this behaviour comes from an operation the user should not do, which is doing savepath() with undesired paths. However, since several people found this behaviour, should it be taken care of in PhysIO directly ? I mean to fix this only takes one line... Should I prepare a PR for this ?

Best, Benoît

mrikasper commented 2 years ago

Dear Benoît,

it's definitely something that PhysIO should check and make the user aware of. I am not sure how exactly you want to fix it. I think the cleanest would be if PhysIO just throws an error mentioned the offending sub-folder.

Changing the path directly might be a bit problematic, because it's a side effect users might not expect. Plus, it wouldn't work in a compiled version of SPM+PhysIO, because path operations cannot be used after compilation (the program always sees all compiled functions).

I would be very happy if you tackle this issue, maybe you can use some of the preexisting code in the toolbox:

Let me know what you think!

All the best, Lars

benoitberanger commented 2 years ago

Hi Lars,

My idea did not involve changing paths, but something rather simple.

As far as I can see, window function is only used in this function tapas_physio_filter_respiratory, so what I propose is simple. Changing

padding_window = window(@blackmanharris, 2 * n_pad + 1);

Into

padding_window = window(@blackmanharris, 2 * n_pad + 1);
padding_window = padding_window(:); % make sure to have a column vector, cf. https://github.com/translationalneuromodeling/tapas/issues/166

This will solve the this particular local problem, and is compatible with a compiled version, right ?

At some point, if PhysIO toolbox needs to use window function another time, a new function could be created such tapas_physio_window to encapsulate the 'window' +' to_column' steps.

I agree this change will not fix the problem if a user has in it's path another window function that shadows the built-in one. But this would be a different problem...

What do you think ?

Best, Benoît

mrikasper commented 2 years ago

Dear Benoît,

I see. I also tried the reshaping of the array last week, but unfortunately, you will run into another function (I think filtfilt) that is called from the fieldtrip folder. This shadowing is even worse, because no error occurs, but the output (filtered respiratory timeseries) is highly oscillatory and the final respiratory RETROICOR regressors are just flat.

I am afraid that there is a couple of places in the PhysIO code that rely on proper behavior of Matlab signal processing toolbox functions, and it would be some effort to change all of them consistently. So I believe, for now, it's probably better to do sth. as suggested above to put an obvious error if relevant functions might be shadowed in the path.

All the best, Lars

benoitberanger commented 2 years ago

Hi Lars,

I see... after testing, I just came across the same problem with filtfilt Collisions and shadowing of function name is a nightmare...

TAPAS/PhysIO did the right choice by using a proper naming convention (tapas_ / tapasphysio), and I think this is the good direction. tapas_physio_window and tapas_physio_filtfilt sounds good, but this takes time to code properly, and add UnitTest for long-term reliability. However, this strategy helps resolving the dependency from Signal Toolbox.

One middle ground solution would to be still create tapas_physio_window, tapas_physio_filtfilt that will call explicitly window and filtfiltform Signal Toolbox, whatever there is in the path. What do you think ? This is also viable for the long-term, since the content of tapas_physio_window and tapas_physio_filtfilt can change without breaking the whole processing.

Let me know.

Best, Benoît

mrikasper commented 2 years ago

Dear Benoît,

I have seen examples of temporarily changing the Matlab path (by removing the shadowing one and re-adding it after the function was called) for what you want to achieve. That could definitely work within the tapas_physio_window and tapas_physio_filtfilt functions you suggest. It won't work when compiling versions of SPM+PhysIO, but I anyway know that I have to remove the fieldtrip paths for that, but you would have to include a if ~(isdeployed || ismcc) condition for any path-specific operation you include so that the compiler doesn't complain about addpath/rmpath.

Mind you, I haven't tested all parts of PhysIO with respect to what functional fieldtrip or others could be shadowing, so maybe it's worthwhile to do a quick search what toolbox functions are used within PhysIO that could be affected. One way to do this is via matlab.codetools.requiredFilesAndProducts('tapas_physio_main_create_regressors.m'). Or one could just do a text-search for all the functions in the fieldtrip/external subfolders to see which ones are used by PhysIO.

In the end, it might still be more efficient to change the path at the beginning of the PhysIO-main function removing fieldtrip (this is literally the only collision reason I have encountered in all these years), and putting it back in after the function completes or errors. I am not sure whether one can check for all possible collisions, but it this could be efficiently done, we could just maintain a list of functions that we check collisions for at the beginning of a PhysIO run. Of course, this wouldn't be as fail proof as creating a wrapper function for each candidate (as you suggested), if users call PhysIO functions outside tapas_physio_main_create_regressors. But it would be much less work and later maintenance than wrapping all individual functions.

All the best, Lars

gllmflndn commented 2 years ago

There has been changes in both SPM and FieldTrip to mitigate this: https://github.com/spm/spm/commit/f1f1319 https://github.com/fieldtrip/fieldtrip/issues/1941 Let me know if there are any remaining issues that we should tackle.

mrikasper commented 2 years ago

Dear @gllmflndn ,

Thank you for addressing this issue within SPM and also communicating with the fieldtrip developers to fix it.

I will check it out for the next PhysIO release in March, if there are any remaining problems.

All the best, Lars