search

vistalab / vistasoft

VISTASOFT is the main software repository of the VISTA lab at Stanford University.
http://vistalab.stanford.edu
148 stars 141 forks source link

Can't run mrAnatLoadSpmVol in deployed mode #278

Open soichih opened 6 years ago

soichih commented 6 years ago

We are running vistasoft in deployed (compiled) mode, and we are having an issue with mrAnatLoadSpmVol function.

Error using spm>spm_version (line 1269)
Can't obtain SPM Revision information.

Error in spm (line 875)

Error in mrAnatLoadSpmVol (line 32)

We are using spm8 and I think this is really a spm issue, but I am not sure if spm8 is still maintained or not.. So the easiest fix for us is to patch mrAnatLoadSpmVol with following.

diff --git a/mrAnatomy/VolumeUtilities/mrAnatLoadSpmVol.m b/mrAnatomy/VolumeUtilities/mrAnatLoadSpmVol.m
index 0fed6d6..eba7e1a 100644
--- a/mrAnatomy/VolumeUtilities/mrAnatLoadSpmVol.m
+++ b/mrAnatomy/VolumeUtilities/mrAnatLoadSpmVol.m
@@ -29,7 +29,7 @@ elseif(isstruct(fname)&&isfield(fname,'nifti_type'))
         elseif(strcmp(type,'double')), type = 'float64'; end
         type = spm_type(type);
     end
-    if(strcmp(spm('Ver'),'SPM2'))
+    if(strcmp('SPM8','SPM2'))
         V.dim = [V.dim type];
     else
         V.dt = [type 0];

Unless vistasoft is still supporting spm2, I was wondering if it can simply drop spm2 unless people are still using it?

francopestilli commented 6 years ago

@jyeatman @JWinawer @wandell would it be OK to remove the check for SPM2?

JWinawer commented 6 years ago

Yes, should be ok. I use SPM 12 with vistasoft.

But I think this line won't work:

Since it will always be false.

Meantime, I will check whether any of the validation functions call mrAnatLoadSpmVol.

On Thu, Feb 15, 2018 at 8:23 AM, Franco Pestilli notifications@github.com wrote:

@jyeatman https://github.com/jyeatman @JWinawer https://github.com/jwinawer @wandell https://github.com/wandell would it be OK to remove the check for SPM2?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vistalab/vistasoft/issues/278#issuecomment-365925816, or mute the thread https://github.com/notifications/unsubscribe-auth/ACBX3uqz80mt5IXe8HRV6LYCL4vwDYFVks5tVC_igaJpZM4SG1Qn .

-- Jonathan Winawer Assistant Professor of Psychology and Neural Science

New York University 6 Washington Place New York, NY, 10003 (212) 998-7922 (phone) (212) 995-4018 (fax) jonathan.winawer@nyu.edu http://psych.nyu.edu/winawer/

JWinawer commented 6 years ago

As far as I can tell, this function is not called in any of the validation code and is only called in these two places in the repository:

If the desired functionality for any SPM version beyond 2 is

V.dt = [type 0];

then I suggest eliminating the check, i.e. replacing this block,

if(strcmp(spm('Ver'),'SPM2'))

    V.dim = [V.dim type];

else

    V.dt = [type 0];

end

with

V.dt = [type 0];

since SPM 2 is way out of date and need not be supported.

-Jon

On Thu, Feb 15, 2018 at 8:39 AM, Jonathan A Winawer < jonathan.winawer@nyu.edu> wrote:

Yes, should be ok. I use SPM 12 with vistasoft.

But I think this line won't work:

  • if(strcmp('SPM8','SPM2'))

Since it will always be false.

Meantime, I will check whether any of the validation functions call mrAnatLoadSpmVol.

On Thu, Feb 15, 2018 at 8:23 AM, Franco Pestilli <notifications@github.com

wrote:

@jyeatman https://github.com/jyeatman @JWinawer https://github.com/jwinawer @wandell https://github.com/wandell would it be OK to remove the check for SPM2?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vistalab/vistasoft/issues/278#issuecomment-365925816, or mute the thread https://github.com/notifications/unsubscribe-auth/ACBX3uqz80mt5IXe8HRV6LYCL4vwDYFVks5tVC_igaJpZM4SG1Qn .

-- Jonathan Winawer Assistant Professor of Psychology and Neural Science

New York University 6 Washington Place New York, NY, 10003 (212) 998-7922 (phone) (212) 995-4018 (fax) jonathan.winawer@nyu.edu http://psych.nyu.edu/winawer/

-- Jonathan Winawer Assistant Professor of Psychology and Neural Science

New York University 6 Washington Place New York, NY, 10003 (212) 998-7922 (phone) (212) 995-4018 (fax) jonathan.winawer@nyu.edu http://psych.nyu.edu/winawer/

jyeatman commented 6 years ago

AFQ uses this function and is currently only compatible with spm8. Shouldn’t be hard to update but I’m not sure when I’ll have the time to make that change. Is keeping som8 compatibility a problem?

On Thu, Feb 15, 2018 at 6:18 AM Jonathan Winawer notifications@github.com wrote:

As far as I can tell, this function is not called in any of the validation code and is only called in these two places in the repository:

  • mrAnatComputeSpmSpatialNorm.m 54 Vtemplate = mrAnatLoadSpmVol(templateFileName);
  • dtiLoadNormalizedMap.m 49 V = mrAnatLoadSpmVol(fullfile(p,fname));

If the desired functionality for any SPM version beyond 2 is

V.dt = [type 0];

then I suggest eliminating the check, i.e. replacing this block,

if(strcmp(spm('Ver'),'SPM2'))

V.dim = [V.dim type];

else

V.dt = [type 0];

end

with

V.dt = [type 0];

since SPM 2 is way out of date and need not be supported.

-Jon

On Thu, Feb 15, 2018 at 8:39 AM, Jonathan A Winawer < jonathan.winawer@nyu.edu> wrote:

Yes, should be ok. I use SPM 12 with vistasoft.

But I think this line won't work:

  • if(strcmp('SPM8','SPM2'))

Since it will always be false.

Meantime, I will check whether any of the validation functions call mrAnatLoadSpmVol.

On Thu, Feb 15, 2018 at 8:23 AM, Franco Pestilli < notifications@github.com

wrote:

@jyeatman https://github.com/jyeatman @JWinawer https://github.com/jwinawer @wandell https://github.com/wandell would it be OK to remove the check for SPM2?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/vistalab/vistasoft/issues/278#issuecomment-365925816>, or mute the thread < https://github.com/notifications/unsubscribe-auth/ACBX3uqz80mt5IXe8HRV6LYCL4vwDYFVks5tVC_igaJpZM4SG1Qn

.

-- Jonathan Winawer Assistant Professor of Psychology and Neural Science

New York University 6 Washington Place https://maps.google.com/?q=6+Washington+Place+%0D+%3E+New+York,+NY,+10003&entry=gmail&source=g

https://maps.google.com/?q=6+Washington+Place+%0D+%3E+New+York,+NY,+10003&entry=gmail&source=g> New York, NY, 10003 https://maps.google.com/?q=6+Washington+Place+%0D+%3E+New+York,+NY,+10003&entry=gmail&source=g

(212) 998-7922 (phone) (212) 995-4018 (fax) jonathan.winawer@nyu.edu http://psych.nyu.edu/winawer/

-- Jonathan Winawer Assistant Professor of Psychology and Neural Science

New York University 6 Washington Place https://maps.google.com/?q=6+Washington+Place+%0D+New+York,+NY,+10003&entry=gmail&source=g New York, NY, 10003 https://maps.google.com/?q=6+Washington+Place+%0D+New+York,+NY,+10003&entry=gmail&source=g (212) 998-7922 (phone) (212) 995-4018 (fax) jonathan.winawer@nyu.edu http://psych.nyu.edu/winawer/

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vistalab/vistasoft/issues/278#issuecomment-365939807, or mute the thread https://github.com/notifications/unsubscribe-auth/ABsYyi5hwMSh39AJnB_gRdnvAPe3ZRLeks5tVDx0gaJpZM4SG1Qn .

-- Sent from a mobile device. Please excuse typos

JWinawer commented 6 years ago

I think SPM 8 is fine. It's just SPM 2 that I thought we should stop supporting.

If you have unit testing for AFQ, then maybe you can just check that this change will not cause a problem.

The change I suggested was to replace the if/then/else with the simplest line that would work:

On Thu, Feb 15, 2018 at 10:59 AM, Jason D. Yeatman <notifications@github.com

wrote:

AFQ uses this function and is currently only compatible with spm8. Shouldn’t be hard to update but I’m not sure when I’ll have the time to make that change. Is keeping som8 compatibility a problem?

On Thu, Feb 15, 2018 at 6:18 AM Jonathan Winawer <notifications@github.com

wrote:

As far as I can tell, this function is not called in any of the validation code and is only called in these two places in the repository:

  • mrAnatComputeSpmSpatialNorm.m 54 Vtemplate = mrAnatLoadSpmVol(templateFileName);
  • dtiLoadNormalizedMap.m 49 V = mrAnatLoadSpmVol(fullfile(p,fname));

If the desired functionality for any SPM version beyond 2 is

V.dt = [type 0];

then I suggest eliminating the check, i.e. replacing this block,

if(strcmp(spm('Ver'),'SPM2'))

V.dim = [V.dim type];

else

V.dt = [type 0];

end

with

V.dt = [type 0];

since SPM 2 is way out of date and need not be supported.

-Jon

On Thu, Feb 15, 2018 at 8:39 AM, Jonathan A Winawer < jonathan.winawer@nyu.edu> wrote:

Yes, should be ok. I use SPM 12 with vistasoft.

But I think this line won't work:

  • if(strcmp('SPM8','SPM2'))

Since it will always be false.

Meantime, I will check whether any of the validation functions call mrAnatLoadSpmVol.

On Thu, Feb 15, 2018 at 8:23 AM, Franco Pestilli < notifications@github.com

wrote:

@jyeatman https://github.com/jyeatman @JWinawer https://github.com/jwinawer @wandell https://github.com/wandell would it be OK to remove the check for SPM2?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/vistalab/vistasoft/issues/278#issuecomment-365925816 , or mute the thread < https://github.com/notifications/unsubscribe-auth/ ACBX3uqz80mt5IXe8HRV6LYCL4vwDYFVks5tVC_igaJpZM4SG1Qn

.

-- Jonathan Winawer Assistant Professor of Psychology and Neural Science

New York University 6 Washington Place https://maps.google.com/?q=6+Washington+Place&entry=gmail&source=g https://maps.google.com/?q=6+Washington+Place+%0D+%3E+New+ York,+NY,+10003&entry=gmail&source=g

https://maps.google.com/?q=6+Washington+Place+%0D+%3E+New+ York,+NY,+10003&entry=gmail&source=g> New York, NY, 10003 https://maps.google.com/?q=6+Washington+Place+%0D+%3E+New+ York,+NY,+10003&entry=gmail&source=g

(212) 998-7922 (phone) (212) 995-4018 (fax) jonathan.winawer@nyu.edu http://psych.nyu.edu/winawer/

-- Jonathan Winawer Assistant Professor of Psychology and Neural Science

New York University 6 Washington Place https://maps.google.com/?q=6+Washington+Place&entry=gmail&source=g https://maps.google.com/?q=6+Washington+Place+%0D+New+York, +NY,+10003&entry=gmail&source=g New York, NY, 10003 https://maps.google.com/?q=6+Washington+Place+%0D+New+York, +NY,+10003&entry=gmail&source=g (212) 998-7922 (phone) (212) 995-4018 (fax) jonathan.winawer@nyu.edu http://psych.nyu.edu/winawer/

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/vistalab/vistasoft/issues/278#issuecomment-365939807 , or mute the thread https://github.com/notifications/unsubscribe-auth/ABsYyi5hwMSh39AJnB_ gRdnvAPe3ZRLeks5tVDx0gaJpZM4SG1Qn .

-- Sent from a mobile device. Please excuse typos

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vistalab/vistasoft/issues/278#issuecomment-365972138, or mute the thread https://github.com/notifications/unsubscribe-auth/ACBX3ubKcv2TCz7ojARJ72KO9ZFumgAwks5tVFRVgaJpZM4SG1Qn .

-- Jonathan Winawer Assistant Professor of Psychology and Neural Science

New York University 6 Washington Place New York, NY, 10003 (212) 998-7922 (phone) (212) 995-4018 (fax) jonathan.winawer@nyu.edu http://psych.nyu.edu/winawer/

francopestilli commented 6 years ago

Correct. The issue is with SPM2.

JWinawer commented 6 years ago

@francopestilli , @soichih , @jyeatman : Is this resolved? It's just a short bit of code. We just need someone to test it and commit it, if it hasn't already been done.

soichih commented 6 years ago

I can test it if this is already updated upstream. We are still using our locally patched version of vistasoft.

soichih commented 6 years ago

Can you merge my PR? We are unable to use the official version of vistasoft and our branch is diverging and affecting our workflow.