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 path length option to sholl analysis #1053

Closed
wants to merge 2 commits into from
Closed

Add path length option to sholl analysis #1053

wants to merge 2 commits into from

Conversation

arnaudon
Copy link
Contributor

@arnaudon arnaudon commented Sep 19, 2022

By default the Sholl analysis is done wrt radial euclidean distance, but having path distance as an optional metric for Sholl analysis is sometimes useful.
This PR provides such functionality via an additional parameters distance_type.

@eleftherioszisis
Copy link
Contributor

eleftherioszisis commented Sep 20, 2022

LGTM. Maybe @andrisecker has some comments?

@arnaudon
Copy link
Contributor Author

He is trying it, once he's happy with the result, we can merge!

@eleftherioszisis eleftherioszisis mentioned this pull request Sep 20, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (0ca89b0) to head (262b851).
Report is 42 commits behind head on master.

❗ Current head 262b851 differs from pull request most recent head b73a08a. Consider uploading reports for the commit b73a08a to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1053   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           36        36           
  Lines         2421      2455   +34     
=========================================
+ Hits          2421      2455   +34     

@arnaudon
Copy link
Contributor Author

@andrisecker , are you good with this? Is it doing what you wanted?

@arnaudon
Copy link
Contributor Author

@andrisecker are you still using this? do you think its worth merging or not?

@andrisecker
Copy link

@arnaudon I'm not using it any more, but I think if I had this use case and it's already implemented then you should merge it.

@arnaudon
Copy link
Contributor Author

@eleftherioszisis I let you double check and approve this when you have time, as I cannot merge

neurom/features/morphology.py Outdated Show resolved Hide resolved
neurom/features/morphology.py Show resolved Hide resolved
@arnaudon
Copy link
Contributor Author

rebase is a pain, and this feature is not used, I discard..

@arnaudon arnaudon closed this May 14, 2024
@arnaudon arnaudon deleted the path_sholl branch May 14, 2024 08:29
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.

6 participants