NISOx-BDI / SwE-toolbox

SwE toolbox
GNU General Public License v2.0
16 stars 7 forks source link

Avoid chaining calls to 'subsasgn' when writing by indices GIfTI files in Octave #164

Closed BryanGuillaume closed 4 years ago

BryanGuillaume commented 4 years ago

The goal of this PR is to circumvent the bug in Octave reported in issue #163.

This is simply done by detecting in swe_data_write that we are trying to write a GIfTI file by indices in Octave. If this is the case, we do not call spm_data_write to do it, but we explicitly write the data in two steps, preventing spm_data_write to chain two calls of subsasgn.

Testing it on my computer in an octave environment seems to fix the issue.

nicholst commented 4 years ago

This goes beyond my understanding/experience with SPM GIFTI I/O. I am surprised that it seems like, when the smoke clears, you're creating data that you would access as

V.private.private.data{1}.data

but if that's what it should be, and it works, fine with me.

BryanGuillaume commented 4 years ago

Yes, I agree that this seems kind of weird to access the data this way. Nevertheless, that is how SPM is currently accessing the data. The only modification I made is to break down the SPM access style into two steps instead of one like is done in spm_data_read:

            try
                %V.private.cdata(varargin{1}) = reshape(Y,size(varargin{1}));
                V.private.private.data{1}.data(varargin{1}) = reshape(Y,size(varargin{1}));
            catch
                %V.private.cdata(varargin{1}) = reshape(Y,size(varargin{1}))';
                V.private.private.data{1}.data(varargin{1}) = reshape(Y,size(varargin{1}))';
            end

Though, interestingly, we can see two commented lines of code that seems to access the data with only one call of subsasgn. Reading the data this way worked on my computer. However, trying to write data this way yielded an error message:

>> V.private.cdata(1) = 2
Error using single
Conversion to single from file_array is not possible.

Error in gifti/subsasgn (line 101)
                        this.data{n}.data =
                        single(builtin('subsasgn',this.data{n}.data,subs(2:end),A)); 

probably explaining why the access is done using V.private.private.data{1}.data instead. Anyway, as this is the closest way SPM is currently writing GifTI data, I think this is ok to do it this way.

nicholst commented 4 years ago

OK... sounds like it's good to merge.