elastix icon indicating copy to clipboard operation
elastix copied to clipboard

Pull request 52 (ReducedFullSampler) updated

Open N-Dekker opened this issue 1 year ago • 2 comments

  • Brought pull request #52 from @MathiasPolfliet up-to-date with the current revision of the main branch.

Other pull requests from Mathias, currently at https://github.com/SuperElastix/elastix/pulls?q=is%3Apr+is%3Aclosed+author%3AMathiasPolfliet+:

  • pull request 51: #51
  • pull request 56: #56 (appears to have the same code changes as pull request 51)
  • pull request 60: #60

The code of the original pull requests was locally retrieved by git fetch origin pull/ID/head:BRANCH_NAME, as described at GitHub Docs - Checking out pull requests locally. The other two new branches based on those old pull request are:

  • https://github.com/SuperElastix/elastix/tree/MathiasPolfliet/More-functionality-stacktransform
  • https://github.com/SuperElastix/elastix/tree/MathiasPolfliet/StackTransformBendingEnergy

N-Dekker avatar Mar 01 '24 15:03 N-Dekker

Comparing ReducedFullSampler and FullSampler, based on the original code at commit 2ec22d0faa5189f8c27ae489fb97a2ed0f626318 "VER: increased to 4.900 in preparation of release" (2018):

elxReducedFullSampler.h vs elxFullSampler.h

  • 124 vs 123 lines
  • Added ReducedInputImageDimension (unnecessary in this case, it seems)

elxReducedFullSampler.h vs elxFullSampler.h

  • 27 vs 32 lines
  • no relevant changes (just "nothing")

elxReducedFullSampler.cxx vs elxFullSampler.cxx

  • 18 vs 22 lines
  • no relevant changes (just elxInstallMacro)

itkReducedFullSampler.h vs itkFullSampler.h

  • 124 vs 126 lines
  • Added ReducedInputImageDimension and InputImageSizeType
  • No ThreadedGenerateData

itkReducedFullSampler.hxx vs itkFullSampler.hxx

  • 150 vs 246 lines
  • No ThreadedGenerateData
  • Essential added code at the begin of GenerateData:
  InputImageIndexType index = this->GetCroppedInputImageRegion().GetIndex();
  index[ ReducedInputImageDimension ] = 0;
  InputImageSizeType size = this->GetCroppedInputImageRegion().GetSize();
  size[ ReducedInputImageDimension ] = 1;
  InputImageRegionType region;
  region.SetIndex( index );
  region.SetSize( size );
  InputImageIterator iter( inputImage, region );

N-Dekker avatar Mar 03 '24 10:03 N-Dekker

Some comments:

  • There appears a lot of code duplication between this component and FullSampler. Can't ReducedFullSampler be written in terms of FullSampler? The FullSampler has very much been improved recently (commit b7029ed0d571940bf99b9f67cb0752cc93514721, 4d028f73990347066f8223c2f594e8ae9ae333bb etc.). It would have been a pity if those improvements won't be included with ReducedFullSampler.
  • I would rename it "ReducedDimensionFullSampler", on order to be consistent with the existing "ReducedDimensionBSplineInterpolator". OK?
  • Does this suggest a reduced-dimension version for Grid, Random, RandomCoordinate, and RandomSparseMask, as well?

@MathiasPolfliet @mstaring @stefanklein Instead of this proposed new component, "ReducedFullSampler" wouldn't it be better to just add support for a new parameter, (ReduceDimension "true"), to the existing FullSampler? And possibly also to other existing sampler components?

N-Dekker avatar Mar 03 '24 10:03 N-Dekker