Skip to content
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: added benchmark for ITK ResampleImageFilter #56

Merged
merged 2 commits into from
Sep 18, 2018

Conversation

dpshamonin
Copy link
Contributor

The benchmark includes support for 1D/2D/3D images,
common interpolators (Linear, Nearest, BSpline),
common registration transforms and their combinations
(Affine, BSpline, Euler, Similarity, Translation, Identity)
and other options to fully benchmark Resample for registration tasks.

The benchmark includes support for 1D/2D/3D images,
common interpolators (Linear, Nearest, BSpline),
common registration transforms and their combinations
(Affine, BSpline, Euler, Similarity, Translation, Identity)
and other options to fully benchmark Resample for registration tasks.
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Excellent work!

//
// Command line argument "-i" "Linear" ["Nearest"] ["BSpline"]
// Command line argument "-soi" controls spline order interpolator for the BSpline interpolator.
// For "-i" "BSpline" the 0th - 5th order splines are supported, the default 3th.
Copy link
Member

Choose a reason for hiding this comment

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

3th -> 3rd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will fix the typos.

}

//------------------------------------------------------------------------------
// This helper function completely define the transform[s]
Copy link
Member

Choose a reason for hiding this comment

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

define: defines ?

}

//------------------------------------------------------------------------------
// This helper function completely set the single transform
Copy link
Member

Choose a reason for hiding this comment

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

set: sets ?

Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

This is awesome @dpshamonin !

As per the ITK style (section C of the Appendix in [1]), I'd rather:

  • Align the loop/conditional statements to the curly brackets.
  • Try to remove the white spaces before the loop/conditional statements and the brackets.
  • Remove the unnecessary
//---------------------------------------

comment lines.

  • In order to embrace C++11, use range-based loops.
  • Although, this has also been on my ToDo list for a long, long time, many of the main program's argument processing/checking functions could be re-used by other testing/benchmarking code, so I guess one day or another it would be useful to create a utils class to gather all this.

Thanks.

[1] https://itk.org/ItkSoftwareGuide.pdf

Copy link
Member

@blowekamp blowekamp left a comment

Choose a reason for hiding this comment

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

Looks good enough to me!

@dpshamonin
Copy link
Contributor Author

@jhlegarreta
Thanks for your kind words. I will fix the typos you found and remove the dashed lines. We already using range based loops when they were suggested by Visual Assist. For the general ITK style, I just used the defined .clang-format from ITK itself, but I had to fix formatting afterwards, since Clang format had many issues formatting this code.

@phcerdan
Copy link
Contributor

Great! ✨

@jhlegarreta
Copy link
Member

Thanks Dennis.

@thewtex
Copy link
Member

thewtex commented Sep 18, 2018

🥇 Thanks, @dpshamonin !

@thewtex thewtex merged commit 09e4aae into InsightSoftwareConsortium:master Sep 18, 2018
@phcerdan
Copy link
Contributor

Great! this is awesome, but now the filtering label takes quite a bit of time.

Label Time Summary:
Core                       =   0.03 sec*proc (1 test)
Filtering                  = 285.63 sec*proc (65 tests)
PerformanceBenchmarking    =   0.19 sec*proc (6 tests)
Registration               =   8.38 sec*proc (3 tests)
Segmentation               =  27.36 sec*proc (4 tests)

Should we create a separate label and CMake option for for Resample? Opened #59

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants