JoramSoch / MACS

MACS – a new SPM toolbox for model assessment, comparison and selection
GNU General Public License v3.0
23 stars 6 forks source link

save() invocations for SPM.mat break with SPM conventions #9

Closed bogpetre closed 2 years ago

bogpetre commented 2 years ago

when spm calls save for SPM.mat matrices it incorporates a version argument defined in spm_defaults.m by calling spm_get_defaults() like so,

save('SPM.mat','SPM', spm_get_defaults('mat.format'));

When the MACS toolbox saves SPM.mat files it does not include this version parameter,

save('SPM.mat','SPM');

This causes MACS to corrupt SPM.mat files when operating on files larger than 2G which cannot be saved by default by save() without the -v7.3 argument included. In these circumstances SPM allows users to change how save behaves by modifying their spm_defaults.m file, but there is no way to transfer this behavior over to the MACS toolbox. The following files and lines should be updated by adding the spm_get_defaults('mat.format') as a 3rd argument to save invocations for SPM.mat,

MA_classic_ICs.m:save(strcat(SPM.swd,'/','SPM.mat')); MA_cvLME_multi.m:save(strcat(SPM.swd,'/','SPM.mat'),'SPM'); MA_cvLME_other.m:save(strcat(SPM.swd,'/','SPM.mat'),'SPM'); MA_cvLME_single.m:save(strcat(SPM.swd,'/','SPM.mat'),'SPM'); MA_inspect_GoF.m: save(strcat(SPM.swd,'/','SPM.mat'),'SPM');

I've attached a patch I attempted to create. I don't have a ton of experience making patches, and I had to change the paths to something generic, and the extensions from .txt to .patch to conform with github's upload requirements, save_patch.txt but at the very least it should make clear what, where and how things need to be changed.

JoramSoch commented 2 years ago

Dear @bogpetre,

thanks for raising this issue! I don't know how to implement such a patch file. But would you mind (i) cloning the repository, (ii) applying the listed changes and (iii) issueing a pull request to me. Then, I can integrate your changes with one or two clicks and you get the proper credit as a contributor.

Cheers, Joram.

lukasvo76 commented 2 years ago

@bogpetre Do you think this may be the cause of the problem I bring up in issue #10?

In any case, it should be easy to implement @bogpetre's solution regardless of whether this is causing my problem or not?

bogpetre commented 2 years ago

Probably unrelated. My issue is pretty minor. I’d check upstream processes. They’re likely throwing some error and then later subsequent steps run and can’t find the files they expect due to the prior error and throw the file not found error.

Bogdan Petre

On Jun 28, 2022, at 18:41, Lukas Van Oudenhove @.***> wrote:

 @bogpetre Do you think this may be the cause of the problem I bring up in issue #10?

In any case, it should be easy to implement @bogpetre's solution regardless of whether this is causing my problem or not?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

JoramSoch commented 2 years ago

@bogpetre, do you still plan on committing the changes and issueing a pull request? If not, I would implement your suggested changes in this week or the next.

JoramSoch commented 2 years ago

This was solved with commit 2901d6e.

bogpetre commented 2 years ago

Hi Joram. Sorry for the radio silence. I’ve been traveling and some threads have fallen through the cracks. I’m just seeing the more recent emails on this thread now. Thanks for committing the changes!

Bogdan Petre

On Jul 1, 2022, at 11:49, Joram Soch @.***> wrote:  Closed #9 as completed.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

JoramSoch commented 2 years ago

No worries and thanks for letting me know.