[Insight-developers] ENH patches for the A2D2 PBNRR

[Matt McCormick]

Hi Brad,

Thanks for the reply.

> You have a wide variety of patches on the topic "PrimaryName". I am having
> difficulty reviewing this topic with as many commits as it is, and many of
> them don't appear to be depended on each other.

All the patches on that branch are related, although the DOC and STYLE
patches could be removed without causing dashboard errors.  The other
three need to be applied in the given order for the CDash at Home
associated with each patch to be green.

>
> If it's a new A2D2, it would be best to get it in while there is still time
> to work on refining it. And you have an outstanding record of rapidly
> addressing issue that come up on the dashboard.
>

We only have until tomorrow to apply A2D2 funding, so I do not know if
it is reasonable for the patches to be merged before the impending
release.  However, if there are comments that need to be addressed,
now is definitely the time.

> As for changing the ProcessObject, to support a custom PrimaryName, I am not
> enthused by this change. It seems like it's adding more complication to the
> base class without a huge benefit. With the interwoven commits on this topic
> it's hard to figure out  where it's needed. It appears that this was the
> approached used to address some bugs in the BlockMatchingImageFilter.
> However, I don't see why the standard numbered inputs were not used. It
> appears that all 3 inputs are required, which matches the conventional usage
> of numbered inputs.

There were a number of bugs addressed in the BlockMatchingImageFilter
and PhysicsBasedNonRigidRegistrationMethod including how the inputs
are handled.

As you can see, they try to use named inputs for all of their inputs.
They tried make use of the named ProcessObject input/output
functionality that was added in 4.0, which has not seen extensive tire
kicking.  There are two bugs they discovered:

1) The first (zeroth) input could not be named.
2) The methods Set/GetNumberOfRequiredInputs refers only to the indexed inputs.

1) is addressed here:

  http://review.source.kitware.com/#/c/6234/

2) is addressed here:

 http://review.source.kitware.com/#/c/6238/

They both could be considered BUG: patches.  I considered renaming
SetNumberOfRequiredInputs to SetNumberOfRequiredIndexedInputs, but
simply improving the documentation seemed better for backwards
compatibility reasons.  While 1) does add more code to ProcessObject,
the named inputs change already added considerable complexity   I
think adding a fix that makes the named inputs fully functional is
worthwhile.  Otherwise, we added all that code and overhead for
something that half-works.

Taking current stock of the classes that use itkSetInputMacro, there are only -

  - ConvolutionImageFilterBase : for the KernelImage
  - MaskFeaturePointPointSelectionFilter : for the optional MaskImage
  - MaskedImageToHistogramFilter : for the optional MaskImage
  - ImageRegistrationMethodv4 : for FixedImage and MovingImage, but it
appears to have similar bugs in defining the required inputs

The alternative to the two patches would take these two filters and
ImageRegistrationMethodv4, use indexed inputs for all the required
inputs, and manually write Set/GetFixedImage proxy methods, which will
internally convert them to indexed inputs (which internally get
converted back into named inputs in ProcessObject anyway).  But, I
think this largely defeats the purpose of having named inputs in
ProcessObject.

Please let me know if you agree or disagree, and I can adjust the
classes as needed.

Thanks,
Matt


>
> Sorry for the delayed response,
> Brad
>
>
> ========================================================
>
> Bradley Lowekamp
>
> Medical Science and Computing for
>
> Office of High Performance Computing and Communications
>
> National Library of Medicine
>
> blowekamp at mail.nih.gov
>
>
> On Jun 18, 2012, at 7:16 PM, Matt McCormick wrote:
>
> Hi,
>
> Please consider the following A2D2 patches for consideration in this
> release cycle.  While it is late in the release process, if they are
> merged within the next few days we will still have funding to address
> any issues that come up on the dashboard during the remainder of the
> week.
>
> This patch:
>
>  http://review.source.kitware.com/#/c/6155/
>
> Is a class to perform the BlockMatching/FEM registration based on the
> already merged feature selection class.
>
> This patch:
>
>  http://review.source.kitware.com/#/c/6234/

>
> Is needed to have the inputs named in the former filter as desired and
> let the pipeline update properly.
>
> There are also related bug fixes that were discovered in the process
> of preparing the patch:
>
>  http://review.source.kitware.com/#/c/6238/
>  http://review.source.kitware.com/#/c/6239/
>  http://review.source.kitware.com/#/c/6245/
>
> Thanks,
> Matt
>
>