Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upWIP: Add a boundary tracing algorithm #4853
Conversation
Hello @fbidu! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-08-22 12:57:08 UTC |
Hi @fbidu great to see that you've made a lot of progress! Please tell us if you need some feedback now. You'll need to add a gallery example too since this is a new feature, probably in |
Co-authored-by: Emmanuelle Gouillart <[email protected]>
Co-authored-by: Emmanuelle Gouillart <[email protected]>
Hi @emmanuelle, I've just commited your suggestions. Thanks! Currently, I'm still working on a new implementation of that function. The current one was a nice start and allowed me to write some tests but it needs some work to make it better. I'll be sure to add the examples later on and let you all know when I have something reviewable. Right know I'm still working on grokking the current behavior |
|
||
return backtrack_start, start | ||
|
||
backtrack_start, start = find_boundary_start() |
rfezzani
Jul 28, 2020
Member
The same as for the starting_point
function, the find_boundary_start
is called only once...
The same as for the starting_point
function, the find_boundary_start
is called only once...
current = neighbors_ids[idx] | ||
counter += 1 | ||
|
||
if np.all(current == start) and np.all(backtrack == backtrack_start): |
rfezzani
Jul 28, 2020
Member
Why not directly use this condition to stop the while loop instead of the while True
?..
Why not directly use this condition to stop the while loop instead of the while True
?..
FirefoxMetzger
Aug 27, 2020
Contributor
I don't think so. This looks like a bottom controlled loop to me; moving it to the top would mean we never enter the body given the current initialization.
I don't think so. This looks like a bottom controlled loop to me; moving it to the top would mean we never enter the body given the current initialization.
rfezzani
Aug 27, 2020
•
Member
You simply need to correctly initialize you variables to enter the loop 😉
I see, it's more complicated then it looks =p
You simply need to correctly initialize you variables to enter the loop
I see, it's more complicated then it looks =p
# initilization | ||
# starting point is the most upper left point | ||
idx_start = 0 | ||
while True: # asserting that the starting point is not isolated |
rfezzani
Jul 28, 2020
Member
Suggested change
while True: # asserting that the starting point is not isolated
for start in zip(y, x):
while True: # asserting that the starting point is not isolated | |
for start in zip(y, x): |
y = neighbors_ids[:, 0] | ||
x = neighbors_ids[:, 1] |
rfezzani
Jul 28, 2020
Member
Suggested change
y = neighbors_ids[:, 0]
x = neighbors_ids[:, 1]
y, x = neighbors_ids.T
y = neighbors_ids[:, 0] | |
x = neighbors_ids[:, 1] | |
y, x = neighbors_ids.T |
boundary.append(current) | ||
backtrack = neighbors_ids[idx - 1] | ||
current = neighbors_ids[idx] | ||
counter += 1 |
rfezzani
Jul 28, 2020
Member
I don't see counter
used elsewhere...
Suggested change
counter += 1
I don't see counter
used elsewhere...
counter += 1 |
raise RuntimeError("The backtrack is not on the neighborhood") | ||
|
||
|
||
def trace_boundary(coords): |
rfezzani
Jul 28, 2020
Member
Since the function is not used for tracing, can we find an other name (region_boundary
?)
Since the function is not used for tracing, can we find an other name (region_boundary
?)
lagru
Jul 28, 2020
Member
... or even object_boundary
. "region" brings regionprops
to mind while "object" fit better within the morphological context.
... or even object_boundary
. "region" brings regionprops
to mind while "object" fit better within the morphological context.
Hello everybody! Sorry for the delay, I didn't have much time in the last couple of weeks to work on this PR but I'll do more work today. @rfezzani thanks for your review and you're right on the 'functions for single calls' and so on. What happened is that the code you're seeing right now was mostly copied from another project. The idea right now is to completely rewrite it in order to become more optimizable. Given the fact that I'm not very familiar with this particular algorithm, I did a careful line by line debugging of its behavior while taking notes. Since I knew it would be a few days before I could lay my hands in this code again, I did a small refactoring in order to isolate the main parts I identified and put my (paper-based) notes on the code so it wouldn't get lost. Thank you all for the comments! Have a great weekend |
@fbidu Great that you are still working on this! I haven't forgotten you nevertheless this PR dropped a bit from my radar. I'll try to catch up till Friday. |
Nice job so far! I think Moore's Boundary Tracing will be a nice addition to skimage. I left some comments on the current implementation. |
import numpy as np | ||
|
||
""" | ||
Code copied from https://github.com/machine-shop/deepwings/blob/master/deepwings/method_features_extraction/image_processing.py#L185 |
FirefoxMetzger
Aug 27, 2020
Contributor
I briefly checked out their repo; while it is open-source there is no LICENSE file and I didn't find any other information on the license. Are we allowed to simply copy-paste the code?
License questions always confuse the heck out of me 😅
I briefly checked out their repo; while it is open-source there is no LICENSE file and I didn't find any other information on the license. Are we allowed to simply copy-paste the code?
License questions always confuse the heck out of me
lagru
Aug 30, 2020
Member
It seems that this code was written by one of @stefanv's students so I expect that this won't be a problem. Nevertheless you're right, we should make sure that everyone involved is in agreement and happy. @theobdt is it alright if we reuse your code here? It would probably be easiest if you could license it with a BSD-compatible license or if you explicitly stated your consent here (in case we have it). 🙏
See this comment on zulip by @stefanv.
It seems that this code was written by one of @stefanv's students so I expect that this won't be a problem. Nevertheless you're right, we should make sure that everyone involved is in agreement and happy. @theobdt is it alright if we reuse your code here? It would probably be easiest if you could license it with a BSD-compatible license or if you explicitly stated your consent here (in case we have it).
See this comment on zulip by @stefanv.
theobdt
Aug 31, 2020
Hello @lagru, yes I personally agree with this code being reused, as long as @stefanv is also ok with it. We also worked on an initial Cython version, which might be relevant.
Hello @lagru, yes I personally agree with this code being reused, as long as @stefanv is also ok with it. We also worked on an initial Cython version, which might be relevant.
"""Coordinates of the region's boundary. The region must not have isolated | ||
points. |
FirefoxMetzger
Aug 27, 2020
Contributor
Suggested change
"""Coordinates of the region's boundary. The region must not have isolated
points.
"""Coordinates of the region's boundary. The region must be simply connected.
This algorithm will get problems with more than merely having isolated points. We also can't have separate groups of points; i.e., for each two points in the region there must exist a path that connects them for which each point along the path is within the region. There is a mathematical term for this, but if forgot xD
On top, we will get issues if the region "has holes". For example, if you put in a donut, it can find the outer boundary, but will not touch the inner one. Not sure if we implicitly exclude such shapes. If not, we need simply connected regions (regions that aren't "split" and that have no holes).
"""Coordinates of the region's boundary. The region must not have isolated | |
points. | |
"""Coordinates of the region's boundary. The region must be simply connected. |
This algorithm will get problems with more than merely having isolated points. We also can't have separate groups of points; i.e., for each two points in the region there must exist a path that connects them for which each point along the path is within the region. There is a mathematical term for this, but if forgot xD
On top, we will get issues if the region "has holes". For example, if you put in a donut, it can find the outer boundary, but will not touch the inner one. Not sure if we implicitly exclude such shapes. If not, we need simply connected regions (regions that aren't "split" and that have no holes).
boundary : 2D array | ||
List of coordinates of pixels in the boundary | ||
The first element is the most upper left pixel of the region. | ||
The following coordinates are in clockwise order. |
FirefoxMetzger
Aug 27, 2020
Contributor
I think for now it is fine to default to clock-wise. Long-term, I think an additional parameter clockwise=True
would be a nice addition.
I think for now it is fine to default to clock-wise. Long-term, I think an additional parameter clockwise=True
would be a nice addition.
# initilization | ||
# starting point is the most upper left point | ||
idx_start = 0 | ||
while True: # asserting that the starting point is not isolated | ||
start = [y[idx_start], x[idx_start]] | ||
|
||
# Focus Start is a 3x3 matrix centered around start | ||
focus_start = binary[start[0] - 1 : start[0] + 2, start[1] - 1 : start[1] + 2] | ||
|
||
# If the current pixel is not the only one set to one in its | ||
# 3x3 region, we stop | ||
if np.sum(focus_start) > 1: | ||
break | ||
idx_start += 1 |
FirefoxMetzger
Aug 27, 2020
Contributor
This seems a bit unnecessary. We require that the region doesn't contain isolated points, or - if you agree with my suggestion - that it is simply connected. In this case we are guaranteed to find at least one neighbor that is also part of the region.
An exception (edge case) is if the region only has a single pixel, in which case your current implementation will throw an error because it runs out of bounds (either because of y[idx_start]
or further down when using idx_start
which now is idx_start=len(coords)
.
I would replace this with simply grabbing the coordinate with the lowest indices.
This seems a bit unnecessary. We require that the region doesn't contain isolated points, or - if you agree with my suggestion - that it is simply connected. In this case we are guaranteed to find at least one neighbor that is also part of the region.
An exception (edge case) is if the region only has a single pixel, in which case your current implementation will throw an error because it runs out of bounds (either because of y[idx_start]
or further down when using idx_start
which now is idx_start=len(coords)
.
I would replace this with simply grabbing the coordinate with the lowest indices.
current = start | ||
backtrack = backtrack_start | ||
boundary = [] | ||
counter = 0 |
FirefoxMetzger
Aug 27, 2020
Contributor
Suggested change
counter = 0
Looks like counter isn't used anymore.
counter = 0 |
Looks like counter isn't used anymore.
y = neighbors_ids[:, 0] | ||
x = neighbors_ids[:, 1] | ||
neighbors = binary[tuple([y, x])] |
FirefoxMetzger
Aug 27, 2020
Contributor
You control _moore_neighborhood
; wouldn't it make sense to return neighbors
in a format you can directly use instead of having to first do conversion so that you can use it?
You control _moore_neighborhood
; wouldn't it make sense to return neighbors
in a format you can directly use instead of having to first do conversion so that you can use it?
current = neighbors_ids[idx] | ||
counter += 1 | ||
|
||
if np.all(current == start) and np.all(backtrack == backtrack_start): |
FirefoxMetzger
Aug 27, 2020
Contributor
I don't think so. This looks like a bottom controlled loop to me; moving it to the top would mean we never enter the body given the current initialization.
I don't think so. This looks like a bottom controlled loop to me; moving it to the top would mean we never enter the body given the current initialization.
if np.all(current == start) and np.all(backtrack == backtrack_start): | ||
break | ||
|
||
return np.array(boundary) |
FirefoxMetzger
Aug 27, 2020
Contributor
This is a quite opinion based comment: I would prefer the output would be of the same format as np.nonzero()
. The same goes for measures.regionprops().coords
, but I don't know what the members think about this (@lagru )?
This is a quite opinion based comment: I would prefer the output would be of the same format as np.nonzero()
. The same goes for measures.regionprops().coords
, but I don't know what the members think about this (@lagru )?
Description
This is a work in progress implementation of the boundary tracing algorithm that started at SciPy sprints. It is not ready for review yet
Thread on Zulip
Might close #1131
Checklist
./doc/examples
(new features only)./benchmarks
, if your changes aren't covered by anexisting benchmark
For reviewers
later.
__init__.py
.doc/release/release_dev.rst
.