Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ENH: Add a 3D regression test for FillholeImageFilter #4330

Merged
merged 1 commit into from
Nov 30, 2023
Merged

ENH: Add a 3D regression test for FillholeImageFilter #4330

merged 1 commit into from
Nov 30, 2023

Conversation

dzenanz
Copy link
Member

@dzenanz dzenanz commented Nov 15, 2023 •
edited
Loading

Closes #4326.

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)
  • Added license to new files (if any)

@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Enhancement Improvement of existing methods or implementation area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Filtering Issues affecting the Filtering module labels Nov 15, 2023
@dzenanz dzenanz requested a review from jhlegarreta November 15, 2023 16:26
@dzenanz dzenanz marked this pull request as ready for review November 15, 2023 16:26
@dzenanz
Copy link
Member Author

dzenanz commented Nov 15, 2023

Unfortunately, these tests do not reproduce the problem from #4326.

@dzenanz
Copy link
Member Author

dzenanz commented Nov 21, 2023

This force push is a plain rebase.

@dzenanz
Copy link
Member Author

dzenanz commented Nov 21, 2023

I must have done something wrong in the data upload, as the downloaded files are html pages instead of images:
fillhole
I plan to look into it.

@dzenanz
Copy link
Member Author

dzenanz commented Nov 29, 2023

The tests pass locally now.

jhlegarreta
jhlegarreta reviewed Nov 29, 2023
View reviewed changes
Modules/Filtering/LabelMap/test/itkBinaryFillholeImageFilterTest1.cxx Outdated
return EXIT_FAILURE;
}

using ImageType = itk::Image<short, 2>;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why the dimensionality is hard-coded here if the purpose is to test this with 3D data. Maybe it should be provided as an input argument to the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

This image type is only used to instantiate the reader which will determine the on-disk dimension. The DoIt has the correct dimension.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it is too long since I have not actually read our docs or actively developed, but not sure if I understand: then why is the reader templated? Why don't we put the dimension to e.g. 1 always, regardless of the dimensionality of the image?

Copy link
Member Author

Choose a reason for hiding this comment

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

1 is just as arbitrary as 2. Removing the explicit dimension specification might be better? It would be 2 by default.

Copy link
Member

Choose a reason for hiding this comment

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

Removing the explicit dimension specification might be better?

If it does not have a function, then to me it does not help to have the dimension specification in any ways.

I will need to revise my knowledge about the toolkit if it does not have a function; I cannot review the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

This approach is inspired by: https://github.com/Slicer/Slicer/blob/5e45824d5cc654a8277205921fb4a1ed129167bb/Base/CLI/itkPluginUtilities.h#L17-L30

Copy link
Member

Choose a reason for hiding this comment

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

The way I sometimes get the dimension of a file is just with the ImageIOBase:
https://github.com/SimpleITK/SimpleITK/blob/46bf97cf954646e284a8ebbc6863ac8b2c5465fe/Code/IO/src/sitkImageReaderBase.cxx#L225

@blowekamp blowekamp self-requested a review November 30, 2023 19:38
blowekamp
blowekamp approved these changes Nov 30, 2023
View reviewed changes
Copy link
Member

@blowekamp blowekamp left a comment

Choose a reason for hiding this comment

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

Thanks for adding more testing.

@dzenanz dzenanz merged commit 3d54107 into InsightSoftwareConsortium:master Nov 30, 2023
6 checks passed
@dzenanz dzenanz deleted the fillholeTest branch November 30, 2023 19:44
@dzenanz dzenanz mentioned this pull request Jan 23, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhlegarreta jhlegarreta jhlegarreta left review comments

@blowekamp blowekamp blowekamp approved these changes

Assignees
No one assigned
Labels
area:Filtering Issues affecting the Filtering module area:Python wrapping Python bindings for a class type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fillhole image filter broken in Python for 3D images
3 participants
@dzenanz @blowekamp @jhlegarreta