Skip to content

Conversation

@oesteban
Copy link
Contributor

@oesteban oesteban commented May 1, 2017

Adds a profiling flag that enables parsing of the standard output from ANTS. This first version only reads two values: the total elapsed time and the final value of the metric.

Adds a `profiling` flag that enables parsing of the standard output from ANTS. This first version only reads two values: the total elapsed time and the final value of the metric.
@oesteban oesteban requested a review from hjmjohnson May 1, 2017 01:47
@oesteban oesteban requested a review from satra May 1, 2017 01:56

verbose = traits.Bool(argstr='-v', default=False)
profiling = traits.Bool(False, usedefault=True,
desc='generate profiling output fields')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There does not seem to be any performance penalty on collecting this information - is there a need for this input?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really.

My original idea is that we grow the number of outputs that are parsed from the log. When there are a bunch, there's a good chance that logs formatting change or we make mistakes. Being an added feature (and nonfunctional), it could come handy to have a switch to avoid parsing the stdout.

But I have no concern with removing the input.

inverse_warped_image = File(desc="Outputs the inverse of the warped image")
save_state = File(desc="The saved registration state to be restored")
metric_value = traits.Float(desc='the final value of metric')
elapsed_time = traits.Float(desc='the total elapsed time as reported by ANTs')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a difference between this elapsed_time and duration stored in every CommandLineInterface runtime? If you need this as an output it might be better to enable this for all interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elapsed time is calculated by ANTs so it refers exclusively to the registration process. The duration of the runtime object adds the (little) overhead of nipype. In general, they will be equivalent, but this could be a good handle to catch problems.

Additionally, my plan would be to add the elapsed_time per registration stage at some point. Then, it will make sense to be consistent with ANTs calculations.

@codecov-io
Copy link

codecov-io commented May 2, 2017

Codecov Report

Merging #1985 into master will decrease coverage by 0.01%.
The diff coverage is 31.81%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #1985 +/- ## ========================================= - Coverage 72.31% 72.3% -0.02%  ========================================= Files 1180 1180 Lines 58885 58907 +22 Branches 8474 8480 +6 ========================================= + Hits 42585 42590 +5  - Misses 14921 14936 +15  - Partials 1379 1381 +2
Flag Coverage Δ
#smoketests 72.3% <31.81%> (-0.02%) ⬇️
#unittests 69.86% <31.81%> (-0.04%) ⬇️
Impacted Files Coverage Δ
...pe/interfaces/ants/tests/test_auto_Registration.py 85.71% <ø> (ø) ⬆️
nipype/interfaces/ants/registration.py 72.83% <31.81%> (-2.17%) ⬇️
nipype/utils/profiler.py 40.12% <0%> (-1.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56b7c81...6909149. Read the comment docs.

@oesteban
Copy link
Contributor Author

@chrisfilo good to go?

@chrisgorgo
Copy link
Member

Please add a changelog entry and you can go ahead and merge it.

@oesteban oesteban merged commit 46f0b74 into nipy:master Oct 13, 2017
@oesteban oesteban deleted the enh/antsRegistrationMinorAdd branch October 13, 2017 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants