-
Notifications
You must be signed in to change notification settings - Fork 67
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.
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
WIP: ENH: Add watershed with distance map example. #86
base: master
Are you sure you want to change the base?
WIP: ENH: Add watershed with distance map example. #86
Conversation
Folks, A few notes:
Finally, it looks like the original code is locally failing at the |
0cb6468
to
28c9ccc
Compare
|
||
bubble_image_clamp = clampFilter.Update() | ||
|
||
itk_vol_img = clampFilter.GetOutput() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MultiplyImageFilter
instantiation yields an error for which I did not find much of an explanation (may be the types involved are not wrapped?), apart from this discourse thread.
However, I guess the piece of code between L118 and L132 can be substituted by
itk_vol_img = (itk.GetArrayFromImage(bubble_image)*255.0).clip(0, 255).astype(np.uint8)
I think sticking to ITK filters as much as possible is better, though. I anyone happens to have a solution to the above conundrum, it would be much appreciated. Also, I ignore the downstream effects of either choice.
Even if the array solution is used, the casting and the input image type for the SignedMaurerDistanceMapImageFilter
seem to cause some trouble that for the moment I have not solved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, MultiplyImageFilter
may not be wrapped for those types. We could case the inputs to float with .astype
. In itk-5.2.0.post1, .astype
works on itk.Image
, (internally it uses itk.CastImageFilter
) preserving pixel information.
I will work on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @thewtex thanks 🚀 ! This has been on my plate for a very long time, and I have not forgot about, but have not found the time to invest. So I'd ❤️ to work with you on this and see how far we can push it/if we manage to have it working !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it! :-D
28c9ccc
to
db43062
Compare
@kmader you are welcome to review. Please. |
db43062
to
2ed8b81
Compare
2ed8b81 rebased on master to start off from the wave of examples transitioned from the WikiExamples. |
2ed8b81
to
85ca522
Compare
85ca522 rebased on master to avoid conflicts early on after the last big example import from the WikiExamples has been merged. |
e661d4d
to
024dd10
Compare
This is looking great 👀 |
Not yet Matt. I think the few issues mentioned in this comment still remain, and I am still unable to run Kevin's original script to have a ground-truth to compare to. |
024dd10
to
54e97a2
Compare
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Rebased |
54e97a2
to
0a865e0
Compare
0a865e0: not finished yet/not tested/not formatted using
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is close. Some in-line comments.
src/Segmentation/Watersheds/SegmentWithWatershedAndDistanceMap/Documentation.rst
Outdated
Show resolved
Hide resolved
src/Segmentation/Watersheds/SegmentWithWatershedAndDistanceMap/Code.py
Outdated
Show resolved
Hide resolved
src/Segmentation/Watersheds/SegmentWithWatershedAndDistanceMap/Code.py
Outdated
Show resolved
Hide resolved
src/Segmentation/Watersheds/SegmentWithWatershedAndDistanceMap/Code.py
Outdated
Show resolved
Hide resolved
0a865e0
to
409b6ee
Compare
@thewtex looks like the GitHub lint action using I had formatted locally to 120 😅 , following what has been adopted de facto in Thanks. |
I guess you can follow the example of InsightSoftwareConsortium/ITK#2608, and use 120 character lines. |
src/Segmentation/Watersheds/SegmentWithWatershedAndDistanceMap/Code.py
Outdated
Show resolved
Hide resolved
The thing is that the lint check should be updated, or a |
Sounds easy enough 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding pyproject.toml
-- for example
[tool.black]
line-length = 120
-- to the repository top-level directory will set black
parameters such as line-length for this repository on your local computer, but I am unsure if or how it affects github actions.
...mentation/Watersheds/SegmentWithWatershedAndDistanceMap/segmentation_result_visualization.py
Outdated
Show resolved
Hide resolved
@Leengit |
Shorter line lengths may be needed to prevent content from being cut off in the rendered PDF. |
Not sure how the PDF generation would behave: e.g. the same should have happened to the ITK SW Guide when the C++ line length was set to 120 unless the ITK core examples folder was excluded and the files manually formatted; not sure if the VTK folks use But I've seen the agreement to adopt black's default 88 in the ITK core in the cross-referenced PR, so I will stick to that 👍. |
Examples (which go into the software guide) have a different, lower line length limit: |
Tried to have a go at Kevin's script to be able to reproduce his result and hopefully complete the PR, I am getting the same error reported here: I also tried using the Trying to specify the I do not feel like having the necessary bandwidth to build the wrapping from sources, so following ITK PR 2554, I will wait to the next release to give this another try. I do not see any other solution to this. Thanks. |
Are you using self-built wrappings from master? Or pre-compiled, pip-installed wheels of version 5.2? Edit: I wrote the question after reading the first paragraph 😞 |
Waiting for the next release of Python wheels sounds reasonable. |
I pip-installed 5.2.
👍 |
409b6ee
to
3fbe489
Compare
3fbe489
to
79e1d89
Compare
Gave another go at this after ITK PR 2554 had been merged and shipped into ITK 5.2.1.post1:
raised here. I'd say that the idea is to avoid building ITK from source with all necessary types wrapped to have this working. Any suggestion @dzenanz @thewtex? Thanks. |
First use |
79e1d89
to
03da5e1
Compare
👍 Thanks @dzenanz. Gave it another go. Looks closer than before, but still not fully working:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Judging by https://github.com/InsightSoftwareConsortium/ITK/blob/v5.2.1/Modules/Filtering/ImageFilterBase/wrapping/itkCastImageFilter.wrap#L5-L6 and https://github.com/InsightSoftwareConsortium/ITK/blob/v5.2.1/Modules/Filtering/ImageFilterBase/wrapping/itkCastImageFilter.wrap#L75-L78 I would expect casting from ULL to UL to be available in cast filter. What is the first error message?
|
||
# Cast to unsigned char so that it can be written as a TIFF image | ||
# WatershedImageFilter produces itk.ULL, but CastImageFilter does not wrap | ||
# itk.ULL: see ITK issue 2551 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Judging by https://github.com/InsightSoftwareConsortium/ITK/blob/v5.2.1/Modules/Filtering/Colormap/wrapping/itkScalarToRGBColormapImageFilter.wrap#L9, and default 64 bit identifier type, and that being ULL on Windows,
@dzenanz thanks for spending a thought about this.
I had already noticed that I had also tried casting the watershed image prior to applying the binary morphological operation, e.g.
But despite what you point @dzenanz in the
The binary morphological filter does work (have checked that applying to another image upstream in the pipeline with the right types works), so the output of the watershed seems to be the issue. So I run out of ideas. |
97ce5c5
to
e4d6032
Compare
Add watershed with distance map example. Resolves InsightSoftwareConsortium#48.
e4d6032
to
0ce0e76
Compare
Doing
in the So I'd dare to say that the output of the watershed image filter (?) does not have the right type information under any circumstance (i.e. unrelated to the exampled in this PR). Or else, the binary opening image filter does not correctly infer the type, since the watershed image does have the expected type when being inspected at the IDE/printing its type. Does this make sense? Additionally, having read again Kevin's example, I am not sure whether the binary opening on the watershed would by itself be equivalent to what the original examples seeks to do. So easiest might be to end the |
This is preferable to keeping this PR open indefinitely. But before that, I will give it a crack, maybe even tomorrow. |
The Python test now passes. I also fixed a C++ compile error when legacy is OFF. Can you clean up this contribution so we can get it merged? @jhlegarreta please squash my commit into yours (attribution not necessary). I made it a separate commit so you can clearly see what the fix was. |
@dzenanz Thanks 💯 . Will do that when I get some time. I will need to check the outputs and see if we finally get the whole thing working as in Kevin's notebook ! |
Add watershed with distance map example.
Resolves #48.