Washington-University / HCPpipelines

Processing pipelines for the HCP
https://www.humanconnectome.org/
Other
532 stars 269 forks source link

[Question] Diffusion preprocessing intensity normalization #210

Closed tashrifbillah closed 2 years ago

tashrifbillah commented 3 years ago

Is rescale expected to be a single number or a 3D volume? I experimentally found it to be a single number but then the comments and variable names are misleading.

This line already reduces each volume of a series to a single number. Then, this comment should be somewhat akin to:

extract mean of each b0 of all b0s for the series

and ${basename}_b0_${cnt} should be called ${basename}_b0_mean_${cnt}.

glasserm commented 3 years ago

@MichielCottaar

mharms commented 3 years ago

Does this impact the accuracy of the results in any way? Sure, the author could have named ${basename}_b0_${cnt} differently, or been slightly more precise in the associated comment, but that file gets removed internally to the script , so does it really matter?

tashrifbillah commented 3 years ago

Michael Harms, unfortunately, your comment is not encouraging! I am a scientist who wants to understand what your script is doing rather than using it blindly.

Does this impact the accuracy of the results in any way?

I don't have an answer for you which is why the issue started with the tag [Question]. The confusion got worse because of the fact that fslmaths (v6.0.1) help message does not have the arguments -Xmean -Ymean -Zmean in it. So I do not have a way to confidently verify what your script is doing.

Open-source projects cordially welcome scientists' feedback for improvement. However unlikely, maybe a discussion on a perfunctory question will correct a yet-to-be-discovered bug. So thank you for your cooperation.

coalsont commented 3 years ago

FYI, fslmaths does mention that -Xmean and similar work, but it isn't stated as obviously as listing them all:

...
Dimensionality reduction operations:
  (the "T" can be replaced by X, Y or Z to collapse across a different dimension)
 -Tmean   : mean across time
...
MichielCottaar commented 3 years ago

I cannot fully comment on the original intent of the code as it was written before I joined and I've never asked the original author. I presume it is meant to correct for long-term signal drift by correcting for any differences in the base signal. Anyway, I am confident that rescale is supposed to be a single number. Dividing by a b0 volume would lead to a lot of problems at this stage (not least of which would be the explosion of noise in any voxels outside of the brain).

So, I mainly view this issue as a request to refactor the variable names and comments to clarify what this section is actually doing. This is a reasonable request (it also took me a while to figure it out the first time I read it).

glasserm commented 3 years ago

Thanks Michiel.

tashrifbillah commented 3 years ago

FYI, fslmaths does mention that -Xmean and similar work, but it isn't stated as obviously as listing them all:

Thanks Tim Coalson, I just learned that.

Anyway, I am confident that rescale is supposed to be a single number.

Thanks Michiel Cottaar for confirming my understanding.

This is a reasonable request (it also took me a while to figure it out the first time I read it)

I can contribute to that scope. Would you welcome a pull request?

glasserm commented 3 years ago

Yes we welcome pull requests that improve the HCP Pipelines.

mharms commented 2 years ago

Closing. We will welcome a pull-request to revise the script's internal variable names if you submit one.