ENH: Added policy for constant NeighborhoodRange values outside image

[hjmjohnson]

No description provided.

[hjmjohnson]

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

Uploaded patch set 1.Oct 9 4:30 PM

Kitware Build Robot
Patch Set 1: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23795-1&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 9 4:41 PM

Niels Dekker
Patch Set 1: The next patch set should fix the errors reported by the Build Robot: Win-VS14 itkShapedImageNeighborhoodRange.h(568): error C2440: 'default argument': cannot convert from 'nullptr' to 'const PixelType', and MacOSX-Clang itkConstantBoundaryImageNeighborhoodPixelAccessPolicy.h:99:3: error: exception specification of explicitly defaulted copy constructor does not match the calculated oneOct 9 4:59 PM

Niels Dekker
Uploaded patch set 2.Oct 9 5:04 PM

Kitware Build Robot
Patch Set 2: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23795-2&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 9 5:16 PM

Pablo He...z-Cerdan
Patch Set 2: Hi Niels, the new constant policy is great to have. I have my doubts about providing the extra template parameter in the NeighborhoodRange though just for this. I commented in https://discourse.itk.org/t/custom-border-extrapolation-of-shapedimageneighborhoodrange-by-imageneighborhoodpixelaccesspolicy/879/23Oct 9 6:08 PM

Niels Dekker
Patch Set 2: The next patch set adds the struct EmptyPixelAccessParameter to itkShapedImageNeighborhoodRange.h, specify when there is no pixel access parameter needed for a specific range. I find this more readable than using std::nullptr_t, and it may potentially reduce the memory footprint, as sizeof(EmptyPixelAccessParameter) is typically less than sizeof(std::nullptr_t).Oct 14 9:16 AM

Niels Dekker
Uploaded patch set 3.Oct 14 9:27 AM

Kitware Build Robot
Patch Set 3: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23795-3&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 14 10:08 AM

Niels Dekker
Patch Set 3: The next patch set should work around "error: excess elements in struct initializer" from the old AppleClang 6.0.0.6000056, as reported by the Build Robot.Oct 15 3:05 AM

Niels Dekker
Uploaded patch set 4.Oct 15 3:08 AM

Kitware Build Robot
Patch Set 4: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23795-4&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 15 3:25 AM

Niels Dekker
Patch Set 4: For the record, Matt is trying out this patch set at https://github.com/KitwareMedical/ITKUltrasound/pull/98Oct 18 5:26 AM

Niels Dekker
Uploaded patch set 5.Oct 18 7:46 AM

Niels Dekker
Patch Set 5: Patch Set 5 adds ShapedImageNeighborhoodRange::SetLocation(index), as it seems helpful for Matt's use case at https://github.com/KitwareMedical/ITKUltrasound/pull/98Oct 18 7:48 AM

Kitware Build Robot
Patch Set 5: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23795-5&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 18 9:03 AM

Matt McCormick
Patch Set 5: Awesome, thanks, Neils!Oct 18 10:26 AM

Niels Dekker
Oct 20 8:48 AM
↩
Patch Set 5: Code-Review-1
(1 comment)
The next patch set should have a major performance improvement of operator*(), by taking away the return-statement from the for-loop in the private helper function CalculatePixelIndexValue (see below).
Modules/Core/Common/include/itkConstantBoundaryImageNeighborhoodPixelAccessPolicy.h
Line 85:
It appears that returning out of this for-loop causes a major performance penalty!

Niels Dekker
Uploaded patch set 6.Oct 20 9:04 AM

Kitware Build Robot
Patch Set 6: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23795-6&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 20 10:08 AM

Niels Dekker
Patch Set 6: Th next patch set should fix "warning: unused parameter 'imageSize'", reported by the Build Robot.Oct 20 3:18 PM

Niels Dekker
Uploaded patch set 7.Oct 20 3:20 PM

Kitware Build Robot
Patch Set 7: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23795-7&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 20 4:20 PM

Niels Dekker
Patch Set 7: The next patch set removes the unused NeighborhoodRange data member, m_Image.Oct 21 4:25 AM

Niels Dekker
Uploaded patch set 8.Oct 21 4:28 AM

Kitware Build Robot
Patch Set 8: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23795-8&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 21 4:44 AM

Niels Dekker
Uploaded patch set 9: Patch Set 8 was rebased.Oct 23 5:18 AM

Kitware Build Robot
Patch Set 9: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23795-9&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 23 5:52 AM

Niels Dekker
Uploaded patch set 10.Oct 29 5:04 PM

Niels Dekker
Patch Set 10: FYI, patch set 10 is just a rebase (having fixed two small merge conflicts).Oct 29 5:06 PM

Kitware Build Robot
Patch Set 10: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23795-10&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 29 6:57 PM

Niels Dekker
Uploaded patch set 11.Oct 31 9:45 AM

Kitware Build Robot
Patch Set 11: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23795-11&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 31 11:17 AM

Matt McCormick
Oct 31 1:20 PM
↩
Patch Set 5:
(1 comment)
Modules/Core/Common/include/itkConstantBoundaryImageNeighborhoodPixelAccessPolicy.h
Line 85:
Interesting!

Niels Dekker
Oct 31 4:02 PM
↩
Patch Set 11:
(1 comment)
@matt Another remark on returning out of the for-loop:
Modules/Core/Common/include/itkConstantBoundaryImageNeighborhoodPixelAccessPolicy.h
PS5, Line 85:
Note that I observed this performance penalty only on VS2017. And returning out of a for-loop does not always cause a performance penalty. So it's hard to draw a general conclusion.

Matt McCormick
Patch Set 11: Cool! I am testing with GCC and ICC on Linux with your test case and the ultrasound application, should be interesting...Oct 31 4:20 PM

Niels Dekker
Uploaded patch set 12: Patch Set 11 was rebased.Nov 1 5:48 AM

Kitware Build Robot
Patch Set 12: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23795-12&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Nov 1 7:03 AM

Niels Dekker
Patch Set 12: request build: windowsNov 1 2:01 PM

Kitware Build Robot
Patch Set 12: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23795-12&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Nov 1 2:12 PM

Powered by Gerrit Code Review (2.12-1-g6f7dc21) | Press '?' to view keyboard shortcuts

[N-Dekker]

Yes, I approve this PR, but then of course, I have to admit I am the submitter 😇 Minor remark: I do find the class name a bit lengthy: ConstantBoundaryImageNeighborhoodPixelAccessPolicy But that's no showstopper to me.

[N-Dekker]

@hjmjohnson Just for my understanding, why did you close this PR of mine?

[hjmjohnson]

Accidently deleted/closed branch when the script for moving from Gerrit went haywire.

[hjmjohnson]

@N-Dekker Sorry about the confusion. I tried to bulk push the gerrit repo to my personal repo, and ended up destroying all my PR's in one stroke of the keys. :(. It should be fixed now.

[thewtex]

Some more discussion here:

https://discourse.itk.org/t/custom-border-extrapolation-of-shapedimageneighborhoodrange-by-imageneighborhoodpixelaccesspolicy/879/29

@hjmjohnson or @N-Dekker could this topic please be re-based on master?

[thewtex]

Rebasing on master...

[N-Dekker]

request build: windows ? 😉

[thewtex]

Windows build is a false positive due to a filesystem / network error. 😢 I created Issue #159 to prevent this from happening again.

I will amend and force push to restart the builds. request build: windows does not do anything anymore.

[phcerdan]

Copying from: https://discourse.itk.org/t/custom-border-extrapolation-of-shapedimageneighborhoodrange-by-imageneighborhoodpixelaccesspolicy/879/30

[quote="Niels_Dekker, post:30, topic:879"]
Sorry but I’m not really convinced. Allowing to modify the constant padding value after range initialization might make things harder to follow, in my opinion. Especially when the padding value is modified after retrieving iterators from the range (using begin() and end() ). It would yield the question whether such a modification would still affect these iterators. It seems clearer to me to specify the constant padding value as part of the range initialization so that it would then clearly remain constant.
[/quote]

The issue I see with the current API is that the optional parameter of a particular policy should be encoded in that policy, not in the Range that can use any policy. Right now Range<TPolicy, TOptionalParameterForConstantPolicy>

What would happen if another policy comes by with another parameter type, we will need to modify Range template parameter?

What do you think about this? Do you think another API would be possible?

I suggested the Policy as data member because it solves this mangling, it would be less clean to use, we will have to first construct the range and then modify the policy, but I don't see where the performance will be reduced (there is only one way to know though).

In any case, this is good stuff! 🎩

Requesting changes just to hear if you think there are alternatives.

[N-Dekker]

@phcerdan wrote:

The issue I see with the current API is that the optional parameter of a particular policy should be encoded in that policy, not in the Range that can use any policy. Right now Range<TPolicy, TOptionalParameterForConstantPolicy>

Sorry, I don't get you point. I named the NeighborhoodRange template parameter TOptionalPixelAccessParameter (not TOptionalParameterForConstantPolicy), so it could (optionally) be used for other policies as well.

What would happen if another policy comes by with another parameter type, we will need to modify Range template parameter?

No, the user just has to instantiate the range class with that other parameter type (for example, UserParameterType):

using UserRangeType = ShapedImageNeighborhoodRange<ImageType, UserPolicy, UserParameterType>;

Note that TOptionalPixelAccessParameter might also be a struct or an std::tuple, for example, allowing the user to specify any number of values (as long as supported by the user specified policy).

Does that answer your question?

[phcerdan]

You clarified one, thanks.
Is there any way for not having to have a template parameter: TOptionalPixelAccessParameter in NeighborhoodRange? Any class using policies would need to add two template parameters, the Policy and TOptionalPixelAccessParameter instead of just one (the policy). Aren't you concern about this?

[N-Dekker]

@phcerdan wrote:

You clarified one, thanks.

You're welcome :-)

Is there any way for not having to have a template parameter: TOptionalParameterForPolicies in NeighborhoodRange? Any class using policies would need to add two template parameters, the Policy and TOptionalParameterForPolicies instead of just one (the policy). Aren't you concern about this?

It seems like the most common use cases would not need the pixel access parameter. When a user does not specify the pixel access parameter type when instantiating the Range type, it will be EmptyPixelAccessParameter, by default. And then it will just be ignored.

// OK: TOptionalPixelAccessParameter = EmptyPixelAccessParameter, by default.
using UserRangeType = ShapedImageNeighborhoodRange<ImageType, UserPolicy>;

So I'm not concerned about that. However, I'm interested to know if there are ITK users that would actually need to specify this pixel access parameter at run-time. Otherwise this feature might as well be postponed until after ITK 5.0. Especially if a simple (non-parameterized) ZeroBoundary policy would be sufficient for @thewtex his ultrasound application KitwareMedical/ITKUltrasound#98

[phcerdan]

So I'm not concerned about that.

Relying on default parameters could be problematic for classes needing control on the policies, we suffered this with classes using defaulted boundary conditions.

If this approach is merged, I would recommend that future classes needing control of boundary policies, use the TRange itself as a template parameter. Using the natural TPolicy would be incomplete to define any Range, because missing the optional parameter. The alternative is to use both TPolicy and TOptionalPixelAccessParameter, but my concern again, why a class using a policy should know about the details of the policy in its template parameters, even if it can be defaulted to Empty?

I'm interested to know if there are ITK users that would actually need to specify this pixel access parameter at run-time

Yeah, I don't know, I talked in the past that 4 policies (with no options) would cover 99.9% of the cases, ConstantZero<TImage>, ConstantOne<TImage>, ConstantMax<TImage> and ConstantMin<TImage>. If we don't think run-time is needed, we should stick with the simpler version.

I am just exposing my concerns, I doubt imposing the optional template parameter to all the classes that use policies is the best design, but happy to be convinced in other direction.

[N-Dekker]

@phcerdan wrote:

If this approach is merged, I would recommend that future classes needing control of boundary policies, use the TRange itself as a template parameter. Using the natural TPolicy would be incomplete to define any Range, because missing the optional parameter. The alternative is to use both TPolicy and TOptionalPixelAccessParameter, but my concern again, why a derived class should know about the details of the policy in its template parameters, even if it can be defaulted to Empty?

OK. As a small adjustment to the current patch, a typedef, PixelAccessParameterType, could be added to each policy class. (Policies that don't need them could just do using PixelAccessParameterType = Empty. Or maybe a missing Policy::PixelAccessParameterType could be detected by SFINAE!) Then ShapedImageNeighborhoodRange would no longer need to have that template parameter, TOptionalPixelAccessParameter. Internally it could just use TPolicy::PixelAccessParameterType. Would that be better to you?

Yeah, I don't know, I talked in the past that 4 policies (with no options) would cover 99.9% of the cases, ConstantZero<TImage>, ConstantOne<TImage>, ConstantMax<TImage> and ConstantMin<TImage>. If we don't think run-time is needed, we should stick with the simpler version.

Some user might like to use the average pixel value of an image as its constant padding value. That's certainly an example of a run-time parameter. And it seems like a reasonable use case to me. However, we can also just wait for the user request :-)

[phcerdan]

Then ShapedImageNeighborhoodRange would no longer need to have that template parameter, TOptionalPixelAccessParameter. Internally it could just use TPolicy::PixelAccessParameterType. Would that be better to you?

Yes! 👍

[N-Dekker]

Just some thoughts here on how to implement TPolicy::PixelAccessParameterType:

• Defining an empty struct, as I did in the initial patch (EmptyPixelAccessParameter) might be somewhat cumbersome, especially if it would eventually need to be defined within its own "itkEmptyPixelAccessParameter.h" header file. Instead, it was suggested to use an empty std::tuple<> (at https://stackoverflow.com/questions/16666871/is-empty-struct-defined-by-c-standard)
• C++11 supports checking on an empty struct by std::is_empty<T>.
• The following struct could be used to estimate at compile-time if a policy class actually has a type named PixelAccessParameterType, by checking the Boolean value CheckPolicy<Policy>::HasPixelAccessParameterType:

    template <typename T> struct CheckPolicy
    {
      using YesType = char[2];
      using NoType = char[1];

      template < typename U >
      static YesType& Test(typename U::PixelAccessParameterType*);

      template < typename U >
      static NoType& Test(...);

      static constexpr bool HasPixelAccessParameterType =
        sizeof(Test<T>(nullptr)) == sizeof(YesType);
    };

See https://rextester.com/HVYQ76782

[N-Dekker]

Superseded by #212 (which is from my own fork, instead of the Gerrit/GitHub migration branch by @hjmjohnson). #212 has removed the extra template parameter, TOptionalPixelAccessParameter from the NeighborhoodRange class. Instead, it has added a nested type, PixelAccessParameterType to the proposed ConstantBoundaryImageNeighborhoodPixelAccessPolicy. Please check! @phcerdan @thewtex

[N-Dekker]

@hjmjohnson Can you please close this PR (#105)? It has been superseded by #212 (which has been merged already).