Skip to content

Conversation

@axel-grc
Copy link
Collaborator

@axel-grc axel-grc commented Oct 20, 2025

Close #56

@axel-grc axel-grc force-pushed the pctHoleFillingImageFilter branch from 87ff333 to c42d655 Compare October 21, 2025 07:14
Copy link
Collaborator

@SimonRit SimonRit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, just minor suggestions.

* is reached or an iteration makes no progress.
*/
template <typename TImage>
class HoleFillingImageFilter : public itk::ImageToImageFilter<TImage, TImage>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it / is there a benefit to make it an InPlaceImageFilter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not a good idea to use InPlaceImageFilter when working with snapshots

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a snapshot?

}

// Allocate output as a copy of input
using InitialDuplicatorType = itk::ImageDuplicator<ImageType>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed with an auto keyword.

@axel-grc
Copy link
Collaborator Author

Thanks for the review @SimonRit, I will propose a new version

@axel-grc axel-grc force-pushed the pctHoleFillingImageFilter branch from c42d655 to 40440d2 Compare October 21, 2025 08:26
Copy link
Collaborator

@SimonRit SimonRit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thanks. Should we add a test? Something like create a small constant image, add holes, and check that the result has constant values? We can merge this one first if you'd like.

@SimonRit
Copy link
Collaborator

There is a problem with the git history. If I do

git log --follow include/pctHoleFillingImageFilter.h

the commit presents it as a new file. I would much prefer if you had used

git mv include/SmallHoleFiller.h include/pctHoleFillingImageFilter.h
git mv include/SmallHoleFiller.hxx include/pctHoleFillingImageFilter.hxx

to be able to check the changes. Do you mind changing this? I can do it if you'd like.

@axel-grc
Copy link
Collaborator Author

There is a problem with the git history. If I do

git log --follow include/pctHoleFillingImageFilter.h

the commit presents it as a new file. I would much prefer if you had used

git mv include/SmallHoleFiller.h include/pctHoleFillingImageFilter.h
git mv include/SmallHoleFiller.hxx include/pctHoleFillingImageFilter.hxx

to be able to check the changes. Do you mind changing this? I can do it if you'd like.

I will do it.
And I will add a test in a new commit.

@axel-grc axel-grc force-pushed the pctHoleFillingImageFilter branch 3 times, most recently from 6ee5caa to 5f58cf4 Compare October 23, 2025 15:21
@axel-grc axel-grc force-pushed the pctHoleFillingImageFilter branch 2 times, most recently from 9db8191 to f660561 Compare October 30, 2025 08:49
@SimonRit SimonRit self-requested a review November 13, 2025 09:55
@SimonRit SimonRit force-pushed the pctHoleFillingImageFilter branch from f660561 to fda0792 Compare November 13, 2025 09:56
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.

SmallHoleFilter should be revisited

2 participants