Minor L2Model improvements

[TheChymera]

• updated string formatting syntax to the cleaner .format()
• removed unneeded substitutions (only kept substitutions for which variables are used)
• no longer using scientific number notation (sometimes causes issues with FSL)

[coveralls]

Coverage remained the same at 72.538% when pulling 765333a on TheChymera:l2model into 4b392d5 on nipy:master.

[mwaskom]

'%e' % 1 creates '1.000000e+00', not 1. I'm not sure what happens if you don't pass this as a float in FSL, but I think if you're just making cosmetic changes you should take all efforts to avoid functional changes.

[TheChymera]

Scientific notation is not according to the FSL specifications. I don't know why anybody made the decision to use a float there, especially because /NumPoints cannot possibly be anything except a positive integer.

[mwaskom]

I believe that the original code was based on replicating the files that are produced by FEAT as closely as possible. It's possible these changes are benign or even better justified, but I don't like seeing changes to functionality without a justification (especially since any problems that might be introduced here aren't going to be covered by the unit tests).

[mwaskom]

Also I don't see anything about scientific notation (or anything about these specific lines) in the page you linked to, and I don't think it's the ideal reference for this code because it's describing how to write text files that will be converted to the actual files that FSL uses with an intermediate tool.

[TheChymera]

The intermediate tool is shipped with FSL, so anybody with FSL can check. The resulting file contains no scientific notation:

/NumWaves 4
/NumPoints 6
/Matrix
1 0 0 -6
1 0 0 -3
0 1 0 7
0 1 0 8
0 0 1 -4
0 0 1 -2

Fractions are also not expressed in scientific notation.

[mwaskom]

It might very well be an innocuous change, I am just saying that it is best to avoid changing the function of something that works fine, and if you do want to make those changes, they should be accompanied by a) an argument in favor of them and b) some evidence that they don't disrupt anything.

[TheChymera]

Arguments:

• It uses the appropriate types for the respective parameters - the previousversion does not
• It is easier to read at both the nipype code level as well as at the level of the files it produces - the previous version is considerably more confusing for the casual observer
• It resembles the examples given by FSL - the previous version does not
• It only uses string formatting when it is needed - the previous version seems to make gratuitous use of it

Evidence for it working:

• I am using the newer version - and since it's a one-purpose function it is unlikely there might be any context in which it would fail if it did not already do so.

I would go even further and say that since we are not using the FSL backend in the way FSL expects us to - the old version would be more prone to break in the future.

[satra]

@TheChymera - can you update the CHANGES file? and were you able to find out which version FSL changed to this format?

[TheChymera]

@satra I have only been using FSL for ~2.5 years. But for all that time the format stayed the same. I don't think it is any more probable that FSL at any point used floats and scientific notation for values that can only be integers than it is for our interface to mistakenly have been created with the wrong formatting.

[satra]

here is a counter example from the fsl course:

http://fsl.fmrib.ox.ac.uk/fslcourse/graduate/ica_prac/denoise/denoise.feat/design.con

[satra]

and here (see design.con and design.mat):

http://fsl.fmrib.ox.ac.uk/fslcourse/graduate/analysis/2_seg_prac/vbm/stats/

i have no problem with updating it with the newer format. but we tend to not break backwards compatibility either, if older versions require the scientific notation format.

[TheChymera]

Just heard back from one of the FSL authors:

[...] The FSL vest reading code just uses standard C++ functions for converting strings to floats, so there's no ​_preferred_​ format - it should work with any valid floating point representation ... there was a bug a couple of years back which caused flirt to output its transformation matrices in floating point hexadecimal!

So apparently we were looking for standards when there are none. I would still suggest that:

• numbers which can only be integers should be represented as such - for the sake of legibility and plain common sense.
• for the rest I believe we should stay with scientific notation - since design matrices should be freely scalable (so it makes no sense for the precision to be contingent on the position of the comma, as in fixed point notation).
• we should switch to the newer .format() notation

[satra]

@TheChymera - agree with your suggestions. could you please merge with master and implement your suggestions?

[TheChymera]

@satra ok, so the only required change was to set the COPE number string substitution to integer. For the rest of the values we use whole-number defaults, so we do not need string substitution or special formatting.

should I also run make specs?

[satra]

this might be ok - let's wait for the tests to pass.

[coveralls]

Coverage increased (+0.02%) to 72.28% when pulling 7f5013f on TheChymera:l2model into df8d607 on nipy:master.

[coveralls]

Coverage increased (+0.02%) to 72.28% when pulling 7f5013f on TheChymera:l2model into df8d607 on nipy:master.

[coveralls]

Coverage increased (+0.03%) to 72.284% when pulling 7f5013f on TheChymera:l2model into df8d607 on nipy:master.

[TheChymera]

@satra ?