-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add graph center speedup #164
Conversation
267c9f2
to
571bb1f
Compare
src/skan/csr.py
Outdated
@@ -1001,6 +1002,76 @@ def make_degree_image(skeleton_image): | |||
return degree_image | |||
|
|||
|
|||
def _simplify_graph(graph): |
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 the problem @kushaangupta! This is a very very slow way of doing this. Plus, you're not using weights for the junction-to-junction distance.
skan already implicitly creates the edge list for the graph when using summarize
. You can then do:
from scipy.sparse import coo_matrix
...
table = summarize(skeleton)
src = np.asarray(table['node-id-src'])
dst = np.asarray(table['node-id-dst'])
distance = np.asarray(table['branch-distance'])
n = max(np.max(src), np.max(dst))
g1 = coo_matrix((distance, (src, dst)), shape=(n+1, n+1)).tocsr()
g = g1 + g1.T # make non-directed
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.
You might also want to remap the indices before creating the COO, using skimage.segmentation.relabel_sequential
, for example.
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.
Thanks @jni! I've made the changes as you said and it's finally ready to be called a speedup (when I can resolve the errors on base cases)
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.
What's the current timing comparison on the retina image?
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.
56 seconds is down to 2.6 seconds
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.
Thanks, it solved the problem. But I was wondering since the central node we are finding through this fast method isn't exact same as the one found from the complete method, but a junction or terminal node closest to it.
To improve it, I was wondering about this approach: we can use the center found by the fast method as an initial seed and go along all of this neighbors to see which one is relatively more central (greedily) until we hit another junction or terminal (or maybe when we stop getting neighbors with better centrality).
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.
I think it's better to just leave the approximation. These points don't look to be too far off from the junctions?
scikit-image/scikit-image#6259
The code complexity to me is not worth the small benefit in finding a slightly more central node.
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.
You're right, but I was just considering the base cases of simpler skeletons with maybe no junction nodes. Maybe we can just add a corner case handler for that: if no junction nodes exist in graph then don't simplify?
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.
Yup, that's not bad. I also am not sure what the behaviour is or should be for disconnected skeletons. You should check that (one of the test skeletons is disconnected). Possibly we return the center only of the largest connected component? (scipy.sparse.csgraph.connected_components
)
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.
I added the base case and tests. For multi skeletons, maybe an argument could be added. It could handle them using connected components if argument is "single"
, else would compute individual centers & sholl plots for all?
571bb1f
to
7c30662
Compare
@kushaangupta I think this is basically ready! Could you add a test for it? Maybe using some of the test skeleton data? |
Looks like we need to follow the rest of the ecosystem and drop Py3.7 support, but that is something for a different PR. Thank you @kushaangupta! This is a very exciting development. =D |
Closes #162
Here's initial code for graph center speedup. Although at the moment, on the example image it hardly appears to be speeding up the process of center finding.