Mtnormalise tweaks2

[JRosnarho]

mtnormalise: various changes made to the mtnormalise function

• multi-thread several loops
• Polybasisfunction is no longer 4 separate template structs but now a singular struct with a
  preceding function that defines the number of basis vectors
• run_primitive() has been removed
• added ImageType, MaskType and ValueType aliases
• replaced QR decomposition with a Choleski decomposition
• created a function to perform the Choleski decomposition
• transformed the outlier rejection lambda function into a global function, it returns the mask, the
  initial_mask as well as the combined_tissue values by reference but also the number of voxels
  explicitly
• new condition statement for convergence will initially asks for the number of voxels but will also ask
  for mask comparison should the voxel numbers be the same
• transformed parts of the code into their own separate functions
• also including changes made by @Lestropie which added new options, and changed existing
  options and some of the previous nomenclature

[Lestropie]

• Some of the indentation is different to that used throughout MRtrix3, not that we've properly standardised / enforced such... not too fussed about differences in convention, though there's some errors that should be fixed e.g. lines 276, 277, 283, 315, 316, 322, 402, 555-561.

• Some parameters look like they should be passed via const reference: lines 246, 255, 289

• Function DefineOutput() (line 246) should return void; currently it is returning a copy of the last image created (which is arbitrary), and that output is not utilised anyways (line 363).

  Indeed it's not immediately clear why this function is required: the scratch images never seem to be used...?

• Function RefinedMask() (line 266) would probably make more sense constructing and returning an Image<bool> rather than requiring one be constructed and passed by reference.

• Couple of inline / lambda functions are really long and would benefit from being split across multiple lines.

• Function Choleski() could be moved to a header file in core/math/, and have appropriate assertion checks added.

• @jdtournier: I don't particularly like the keyvalues "lognorm_scale" and "lognorm_balance"; mtnormalise_*" would probably be better?

[thijsdhollander]

Hi Jonas,

Thanks for your efforts to improve the code of mtnormalise! I note some changes seem intended to improve the speed / running time (e.g. multi-threading for different loops). Out of curiosity: did you notice substantial improvements in the effective running time due to these changes?

Other than that, I think you should add yourself to the author list, if you're ok with that yourself of course. Entirely up to you, but I find it 100% justified. Feel free to just go ahead and add yourself. 👍

Finally, if you're pursuing any academic career at all, or if a co-authorship is valuable in any other way, just let me know. No guarantees, but I'll see what I can do.

Cheers,
Thijs

[JRosnarho]

Hello, sorry for the late response on my end I ended up getting quite busy this week. I’ll start of by responding to the points made by @Lestropie concerning specifically the existence of the DefineOutput() function, its purpose and use.

DefineOutput() started as a clunky way of defining the output images at the very beginning of the code, that way if it can define the outputs at the start without crashing then we can continue by computing the values of said outputs and storing them in the already allocated memory. The issue I encountered was that the outputs images could only be created and have their values stored at the end of the code by looping sequentially by each image. Currently DefineOutput() defines the structure needed for each of the output images by creating a variable that will have all the space of the outputs but without the correct values or means to change those values. This means that we still need to remake the outputs at the end but it also means that the outputs are already “defined” in some loose term, which ended up saving a second or two in run time (nothing overly significant).

Hopefully this long winded explanation gives an idea as to what the original intent of the function was, but knowing that it really didn’t accomplish what I had originally planned out it would probably be best to remove it entirely and do any of the small corrections that come from doing so.

Concerning RefinedMask() passing by reference instead of constructing and returning an Image<bool> explicitly I’m not too sure as to why I wrote it in such a manner. And the same can be said to some of the other points, others such as the indentation was just ignorance on my part.

Following onto the points made by @thijsdhollander indeed the intent of the changes were to improve the run time of mtnormalise especially with bigger data sets, which in its original state it ends up taking an exceedingly long time to finish its work. After all the changes that were detailed in the pull request if I ran mtnormalise for a data set that took 2 minutes to run in its original state it now takes around 52 seconds.

Concerning being added to the author list I will not be in a position to modify the code to do so at the current time but @jdtournier has access to my old workstation and I believe he has said that he would add me to the list, which is something that I am not opposed to.

With regards to the last point having a co-authorship is always valuable, even if I do not have plans in the near future to pursue an academic career, it would be very much appreciated on my end.

Thank you both for the remarks/review of the changes and I wish for all of you a pleasant day.

Sincerely,

Jonas Rosnarho-Tornstrand

[thijsdhollander]

Following onto the points made by @thijsdhollander indeed the intent of the changes were to improve the run time of mtnormalise especially with bigger data sets, which in its original state it ends up taking an exceedingly long time to finish its work. After all the changes that were detailed in the pull request if I ran mtnormalise for a data set that took 2 minutes to run in its original state it now takes around 52 seconds.

Nice, that's a solid improvement; definitely worth the effort. Yes, for smaller datasets it is ("was") already reasonably fast, because it's just not all that involved to begin with. Any improvement is always welcome of course. What you're reporting is more than a factor 2; that would certainly be valuable for larger datasets.

Concerning being added to the author list I will not be in a position to modify the code to do so at the current time but @jdtournier has access to my old workstation and I believe he has said that he would add me to the list, which is something that I am not opposed to.

👍 Perfect.

With regards to the last point having a co-authorship is always valuable, even if I do not have plans in the near future to pursue an academic career, it would be very much appreciated on my end.

Ok, I'll take note of it. No guarantees at the moment as to how/when; but that may very well be (very) much sooner rather than later actually. I'm 100% for including young researchers who make an effort like this. 👍

Thanks again!

[Lestropie]

@JRosnarho @jdtournier: Any chance of getting a GitHub avatar image in place for the sake of the changelog & website?