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 n_points methods for immutable Morphology & Vasculature #359

Merged
merged 1 commit into from
Nov 30, 2021

Conversation

eleftherioszisis
Copy link
Contributor

Closes #357

@eleftherioszisis eleftherioszisis changed the title Add n_points methods for immutable Morphology & Vascukature Add n_points methods for immutable Morphology & Vasculature Nov 29, 2021
Copy link
Member

@adrien-berchet adrien-berchet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!
I am just wondering if the property of the Morphology should not be called n_section_points to be more clear?

@eleftherioszisis
Copy link
Contributor Author

Looks good to me, thanks! I am just wondering if the property of the Morphology should not be called n_section_points to be more clear?

Given that there are no other points to be confused within the morphology, isn't it redundant?

@adrien-berchet
Copy link
Member

Looks good to me, thanks! I am just wondering if the property of the Morphology should not be called n_section_points to be more clear?

Given that there are no other points to be confused within the morphology, isn't it redundant?

It's just to make more explicit that the soma points are not counted. But as you prefer, both are good to me, it was just a comment.

@eleftherioszisis
Copy link
Contributor Author

The main reason I prefer n_points is that it reflects the points dataset. Introducing a name with a different infix would make the user think that it might be something more than a counting of the points dataset.

@adrien-berchet
Copy link
Member

Yeah that's why both are good to me, I don't know if one is better than the other for a new user but let's approve like this :)

@mgeplf
Copy link
Contributor

mgeplf commented Nov 29, 2021

I think the point about the naming is important; instead of n_section_points, would n_neurite_points work?

Depending on what underlying file ppl are using, they may consider points and sections to include the soma...

@eleftherioszisis
Copy link
Contributor Author

eleftherioszisis commented Nov 29, 2021

I think the point about the naming is important; instead of n_section_points, would n_neurite_points work?
Depending on what underlying file ppl are using, they may consider points and sections to include the soma...

morphio has no concept of neurites as it works exclusively with sections. Therefore, if a different name is to be used, n_section_points would be more suitable imho. However, as I said above I like how n_points maps intuitively to the points dataset, because otherwise points should be have beed renamed into section_points.

But if having a more specific name would avoid any potential confusion, should I use n_section_points as @adrien-berchet suggested?

@mgeplf
Copy link
Contributor

mgeplf commented Nov 30, 2021

[...] I like how n_points maps intuitively to the points dataset, because otherwise points should be have beed renamed into section_points.

Ok, I see the sense in it, your initial intuition was spot on.

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.

Inexpensive number of points property for Section and Morphology
3 participants