-
-
Notifications
You must be signed in to change notification settings - Fork 384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: antsAtroposN4.sh - exposed more subcommand parameters as arguments #1420
Conversation
Thanks for this. To do:
|
Do you have any docs/experiments on why these Atropos options speed things up/what they do? Atropos is a bit of black box as it has a ton of things not mentioned in the paper which introduces it. |
@gdevenyi That is a very good question :) I would also very much appreciate more information/documentation on what the different Atropos parameters actually affect. Unfortunately I do not have the enough insights or knowledge to fully explain it, but I can provide what insight I have gained during my work. We have been testing out different combinations of parameters and have currently found that the following parameters yield the best trade off between execution speed and output quality for our input images: Atropos parameters "-e 1 -g 0" (for the N4BiasFieldCorrection we use the parameters "-s 4 -c [35x35x35x35,0] -b [200]"). In regards to why the "-g 0" Atropos paramter accelerate the execution speed: This sets the m_UseAsynchronousUpdating=false in the antsAtroposSegmentationImageFilter.hxx filter used by Atropos, which skips the computations inside the if statement in: ANTs/ImageSegmentation/antsAtroposSegmentationImageFilter.hxx Lines 1153 to 1206 in 77b8f2a
In relation to the "-e 1" parameter. As I understand the "Atropos --help" text: Lines 1536 to 1544 in 77b8f2a
and from the code in: ANTs/ImageSegmentation/antsAtroposSegmentationImageFilter.h Lines 611 to 617 in 77b8f2a
the best results are found when "-e 1", which is in line what we have observed. The exact difference in computation between the two can be seen here: ANTs/ImageSegmentation/antsAtroposSegmentationImageFilter.hxx Lines 2433 to 2513 in 77b8f2a
|
Thanks @br-cpvc as I suspected reading the code is still the only way to determine some of the effects here. The comment about the distance calculation is particularly helpful. Cheers. |
@cookpa I initially thought that the "To do list" that you inserted was for your self or someone on your team, but as no updates have been posted I have started to wonder if it was ment for me? Are you waiting for me to do something or what is the current status? |
It's for anyone who has time to do it, you're welcome to take it on if you'd like. Since you have exposed some N4 parameters, it might be nice to add the initial mesh resolution / spacing (currently hard coded as BTW, if you're benchmarking performance, there have been recent changes in ITK that might affect multi-threading performance in N4 (#1017). |
@cookpa thank you for the comment, and thank you for making me aware of the recent patch (#1017) affect multi-threading performance in N4. I have looked into what it takes to align the input parameters between the two scripts. With the current getopts argument parser it seems impossible to add and/or align the input parameters between antsAtroposN4.sh and antsCorticalThickness.sh - this is because of two things:
I think the best will be to make it possible to use long options, but as getopts does not support this, another approach to parsing the arguments must be used. I first looked at the getopt parameter parser, which supports long options. But of what I could find, the getopt package is not recommended, see: http://mywiki.wooledge.org/ComplexOptionParsing the only other option I could find was to implement the parsing directly/manually inside the script, which I have tried based on the example: https://mywiki.wooledge.org/BashFAQ/035#Manual_loop I have redone my implementation based on the current head of the ANTs main branch, and included a new cli option for setting the N4_BSPLINE_PARAMS as you suggested. The reimplementation is currently in master...br-cpvc:ANTs:upstream_20221110_0945 What do you think of the approach? Is this in the right direction or should it be done another way? |
The solution for complex bash parsing is https://argbash.readthedocs.io/en/latest/ and its fantastic. |
Yeah, I'm not sure it's feasible to make every option used within |
@cookpa Ah, okay, then I completely misunderstood what you were after :) I have now taken another go at it. I have pushed another patch to the current PR branch which adds the N4_BSPLINE_PARAMS parameters as you previously suggested. After that, I made a test trying to show that any script that uses antsAtroposN4.sh should expect the same results as before the changes. The test shows that antsAtroposN4.sh produces the same results (it does not alter the output). By executing it two times, before and after the change to the Atropos parameters and their default values (ATROPOS_SEGMENTATION_ICM=1 and ATROPOS_SEGMENTATION_USE_EUCLIDEAN_DISTANCE=0) which were added to the exe_segmentation statement in antsAtroposN4.sh master...br-cpvc:ANTs:upstream_20220906_1020#diff-b35ac52557562254e75ede77f4c181598f938e248bcfcd8d7c4836c7a7d1048dL502-L503 The test:
Execution of the test
Both outputs are exacly the same. I also tried changing the ICM parameter to see that it actually change the results:
What do you think, is this test deep enough to cover what you were asking? |
Yes that looks good to me, thanks. I did a similar test and got identical results with the new script. So I think this is suitable to merge. |
when trying to optimize antsAtroposN4.sh execution speed contra output precision it would be great to expose these internal variables as external cli parameters