search

BIC-MNI / minc-tools

Basic minc-tools from former minc repository
Other
30 stars 25 forks source link

mincaverage behaviour #16

Closed andrewjanke closed 9 years ago

andrewjanke commented 9 years ago

From the MINC mailing list.

Hello, I am running across some unexpected behaviour for mincaverage using -normalize and I'm not sure if it's a bug or not. Description: mincaverage -normalize in1.mnc in2.mnc in3.mnc out.mnc results in out.mnc with all zeros if the input files have a mean less than zero. I can reproduce this every time. At the moment I am able to 'hack' my minc files by adding a constant so that they all have means > 0. I think perhaps the issue might lie in lines 533-561 here: [1]https://github.com/BIC-MNI/minc-tools/blob/master/progs/mincaverage/mincaverage.c Where vol_mean[ifile] is included in the determination of vol_total and global_mean (lines 537 and 554), but the value of average_data.norm_factor[ifile] is conditional upon the vol_mean[ifile] being > 0 (otherwise, the norm_factor[ifile] is set to 0.0). Any ideas if this is a bug (though perhaps not in the lines that I am suggesting) or a design decision? If it's a design decision, can I ask the reasoning? (Maybe I'm not understanding the meaning of world values in minc files?)

Thanks Johnathon

Johnathon R. Walls, PhD Scientist - Therapeutic Target Discovery Regeneron Pharmaceuticals 777 Old Saw Mill River Road Tarrytown, NY USA 10591-6707 Phone: 914.847.3006 Fax: 914.847.7544 Email: johnathon.walls@regeneron.com

andrewjanke commented 9 years ago

Peter Neelins comment:

I think perhaps the issue might lie in lines 533-561 here: [1] https://github.com/BIC-MNI/minc/blob/master/progs/mincaverage/mincaverage.c Where vol_mean[ifile] is included in the determination of vol_total and global_mean (lines 537 and 554), but the value of average_data.norm_factor[ifile] is conditional upon the vol_mean[ifile] being > 0 (otherwise, the norm_factor[ifile] is set to 0.0). Any ideas if this is a bug (though perhaps not in the lines that I am suggesting) or a design decision? If it's a design decision, can I ask the reasoning? (Maybe I'm not understanding the meaning of world values in minc files?)

Looks like a bug to me. Not sure why I would not have tested for == 0.0 (or better, abs(mean) > epsilon) - perhaps I copied the test for sum0 > 0 (which does make sense, since sum0 is a count). Remarkable that no one has found it in the past 17 years.

(Boy, that main() is embarrassingly long! If I were doing a code review I would definitely have something to say to the kid who wrote that!)

rdvincent commented 9 years ago

Minimal fix (and regression test) in e9a891eadb9518c24e9d9830bf72e9ebcb38f081