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.

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 Sholl analysis for Skeleton object #150

Merged
merged 27 commits into from
Feb 23, 2022
Merged

Add Sholl analysis for Skeleton object #150

merged 27 commits into from
Feb 23, 2022

Conversation

jni
Copy link
Owner

@jni jni commented Feb 8, 2022

Supersedes #148

@kushaangupta could you please continue your work in a new branch starting from this one? To add a documentation page, you can write it in myst-nb syntax (or export a notebook to that syntax). We'll update the docs infrastructure in a different PR to make sure that gets rendered correctly in the site.

@kushaangupta
Copy link
Contributor

kushaangupta commented Feb 14, 2022

I was thinking the following changes:

  1. Since the intersection_counts length may not match shell_radii, On line 1214:
intersection_counts = np.bincount(shells, minlength=len(radii)) // 2
  1. To return the computed center. I think that to not to return it is a wasted computational effort. May be it can be added as a property of Skeleton object itself?

@jni
Copy link
Owner Author

jni commented Feb 21, 2022

Thanks for the comments @kushaangupta! Both good suggestions, I've implemented them.

@jni
Copy link
Owner Author

jni commented Feb 22, 2022

Cool, this just needs a couple of tests and is ready to go in I think...!

@kushaangupta
Copy link
Contributor

Oh great work! Sorry, I couldn't work on it. Got a bit confused trying to make tutorial notebook on it.

_path_distances is now not being used given the updated Sholl implementation. Should it be deleted?

@jni jni merged commit 545a9db into main Feb 23, 2022
@jni jni deleted the sholl branch February 23, 2022 07:43
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.

2 participants