Skip to content

Conversation

@dzenanz
Copy link
Member

@dzenanz dzenanz commented Mar 5, 2021

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

@dzenanz dzenanz requested a review from Leengit March 5, 2021 00:53
@dzenanz
Copy link
Member Author

dzenanz commented Mar 5, 2021

This now always fails test 5. Maybe you could take over @Leengit?

@Leengit
Copy link
Contributor

Leengit commented Mar 6, 2021

I've ironed out all the bugs but one. The handy ConstantBoundaryCondition functionality sets a default value for pixels outside of the image, but that's not what we need. We want to be able to set a default value for pixels outside the region being processed. I'll work on that soon. If you want to take a look ... see 0da296e,

@Leengit
Copy link
Contributor

Leengit commented Mar 8, 2021

@dzenanz would you be willing to take another turn at this? The code checked into 9fc6864 is pretty close to done, but I see two issues.

  1. We are copying the input region to be its own image even in the case that it already is all of its image. (Look for '!!!' in itkContourExtractor2DImageFilter.hxx, lines 304-314) I suspect that there is an easy way to check for this condition, but I don't know it.
  2. The iterator loop for the ConstShapedNeighborhoodIterator<InputImageType, BoundaryConditionType> it skips its last element -- it processes exactly 19 of its 5×4 cases. What's going on with that? (Look for '!!!' in itkContourExtractor2DImageFilter.hxx.) In another thread, @thewtex asks whether we could instead use ShapedImageNeighborhoodRange -- I haven't looked at that.

@dzenanz
Copy link
Member Author

dzenanz commented Mar 8, 2021

I could take a turn at this, but probably not this week due to work on another project.

@Leengit
Copy link
Contributor

Leengit commented Mar 8, 2021

I have submitted a competing pull request (#2384) for a number of reasons explained there. Ultimately we'll want to merge only one of these pull requests, but in the meantime we can cherry pick from each other, etc.

@dzenanz
Copy link
Member Author

dzenanz commented Mar 11, 2021

Closing in favor of #2384.

@dzenanz dzenanz closed this Mar 11, 2021
@dzenanz dzenanz deleted the contourExtractorImprovements branch July 9, 2021 14:09
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.

2 participants