<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hello David,<div><br></div><div>Clarifying documentation is certainly a good thing.</div><div>Hello David,</div><div><br></div><div>The NeighthoodOperatorImageFitler is widely used as a base class, changing this type would likely cause a ripple effect across a number of ITK filters and users derived classes. From the the super class' perspective I think the naming convention makes a lot of since, then from the NormalizedCorrelation Filter</div><div><br></div><div>The NormalizedCorrelationImageFilter in particular is not done in frequency space so the cost of correlating images of the same size would be quite extreme. It's more geared to template matching. I imagine that this filter was written with the idea of creating a small template to search for, and having the template and the output both be of real type.</div><div><br></div><div>I however do agree that the name for the operator value type is poor or even misleading. I however don't know if changing the default template parameter will help new users to the class as it may hurt existing users.....</div><div><br></div><div>I think the best thing to do is to clarify the doxygen documentation. You already have the excellent wiki example to demonstrate the usage, so that should go along way with helping people use the class.</div><div><br></div><div>So what I am I saying should be done here... It's not clear to me. It looks like is should be a small patch... perhaps if it's put up for review in gerrit, the +1, and -1 will figure out what's best....</div><div><br></div><div>Brad</div><div><br></div><div><br><div><div>On Jun 26, 2012, at 12:20 PM, David Doria wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div class="gmail_quote">On Tue, Jun 26, 2012 at 9:42 AM, Bradley Lowekamp <span dir="ltr"><<a href="mailto:blowekamp@mail.nih.gov" target="_blank">blowekamp@mail.nih.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word">David,<div><br></div><div>This certainly sounds like a bug.</div><div><br></div><div>Do you have a patch?</div><div><br></div><div>If the patch is not invasive and gets approved today, it may... just may be able to make it into the RC4.</div>
<div><br></div><div>Brad</div></div></blockquote><div><br></div><div>Hi Brad,</div><div><br></div>I think there are actually three separate problems/questions here - </div><div class="gmail_quote"><br></div><div class="gmail_quote">
1)</div><div class="gmail_quote"><div>
I realized that I can specify a 4th template parameter (TOperatorValueType):</div><div><br></div><div><div> typedef itk::NormalizedCorrelationImageFilter<FloatVectorImageType, MaskType,</div><div> FloatImageType, FloatVectorType> CorrelationFilterType;</div>
<div> CorrelationFilterType::Pointer correlationFilter = CorrelationFilterType::New();</div></div><div><br></div><div>However, when I do this, I get a concept check error that ‘itk::CovariantVector<float, 3u> can't be casted to type ‘float’. I'm assuming this means that this filter can only operate on scalar images? I guess now that I think about it, the standard correlation computation doesn't seem to naturally extend to vector-valued pixels... Perhaps there should at least be a mention of this in the documentation, or better, a concept check on TInputImage.</div>
<div><br></div><div>2)</div></div><div class="gmail_quote">Though I don't think I'll be able to use the filter as I was trying to in (1), I still think the variable names and default template parameters are confusing, even for correct usage.</div>
<div class="gmail_quote"><br></div><div class="gmail_quote">In the parent class:<div><br></div><div><div>template< class TInputImage, class TOutputImage, class TOperatorValueType = typename TOutputImage::PixelType ></div>
<div>class ITK_EXPORT NeighborhoodOperatorImageFilter:</div><br></div><div>we see that TOperatorValueType defaults to the output image type. Wouldn't it make more sense to default to the input image type, since you'd usually be correlating two images of the same type? </div>
<div><br></div><div>3)</div><div>With or without the default in (2), the class then goes on to define:</div><div><br></div><div><div> typedef Neighborhood< OperatorValueType,</div><div> itkGetStaticConstMacro(ImageDimension) > OutputNeighborhoodType;</div>
</div><div><br></div><div>should this be renamed OperatorNeighborhoodType - as I'm not sure what it has to do with 'Output'? That would then trickle down to clarify the confusion with the type of the SetTemplate() parameter.</div>
<div><br></div><div>If you agree, I can make a patch for 2 and 3, but 1 probably just needs a comment or a new concept check rather than a patch.</div><div><br></div><div>David</div></div>
</blockquote></div><br><div>
<span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><span class="Apple-style-span" style="border-collapse: separate; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; -webkit-text-decorations-in-effect: none; text-indent: 0px; -webkit-text-size-adjust: auto; text-transform: none; orphans: 2; white-space: normal; widows: 2; word-spacing: 0px; "><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><span class="Apple-style-span" style="border-collapse: separate; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; -webkit-text-decorations-in-effect: none; text-indent: 0px; -webkit-text-size-adjust: auto; text-transform: none; orphans: 2; white-space: normal; widows: 2; word-spacing: 0px; "><p style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" style="font: normal normal normal 12px/normal Helvetica; ">========================================================</font></p><p style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" style="font: normal normal normal 12px/normal Helvetica; ">Bradley Lowekamp<span class="Apple-converted-space"> </span><span class="Apple-converted-space"> </span></font></p><p style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" style="font: normal normal normal 12px/normal Helvetica; ">Medical Science and Computing for</font></p><p style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" style="font: normal normal normal 12px/normal Helvetica; ">Office of High Performance Computing and Communications</font></p><p style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" style="font: normal normal normal 12px/normal Helvetica; ">National Library of Medicine<span class="Apple-converted-space"> </span></font></p><p style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" style="font: normal normal normal 12px/normal Helvetica; "><a href="mailto:blowekamp@mail.nih.gov">blowekamp@mail.nih.gov</a></font></p><br class="Apple-interchange-newline"></span></div></span></span><br class="Apple-interchange-newline">
</div>
<br></div></body></html>