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

Support multiple soma stack in ASC #306

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

asanin-epfl
Copy link
Contributor

@asanin-epfl asanin-epfl commented May 6, 2021

Successor of #39, solution to #300, depends on #294 to update documentation after that.

@asanin-epfl asanin-epfl self-assigned this May 6, 2021
@asanin-epfl asanin-epfl added the WIP Work in Progress label May 6, 2021
@asanin-epfl asanin-epfl removed the WIP Work in Progress label May 7, 2021
@asanin-epfl asanin-epfl requested review from mgeplf and tomdele May 7, 2021 09:25
@asanin-epfl asanin-epfl force-pushed the asc-multiple-soma branch 2 times, most recently from 283d466 to bd570a7 Compare May 11, 2021 07:45
if (!nb_.soma()->points().empty())
throw SomaError(err_.ERROR_SOMA_ALREADY_DEFINED(lex_.line_num()));
nb_.soma()->properties() = properties;
const auto& soma = nb_.soma();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a new SomaType is required for a stack, so it can be distinguished from a simple contour?

Copy link
Contributor Author

@asanin-epfl asanin-epfl May 11, 2021

Choose a reason for hiding this comment

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

I can't say for sure. This type of soma is still a contour. MorphIO does not calculate surface or volume for contour somas. All other properties are not affected by stack. That hesitates me to introduce a new soma type. What for? MorphIO offers access to points and morphology, that's it. I would actually drop surface and volume properties entirely as MorphIO does not calculate them for all cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

That hesitates me to introduce a new soma type. What for?

For one thing, so that a morph read from ASC can be written back to ASC w/ the same structure.

The other problem that I can forsee is the contour might be really strange: if the orientation of the z slices are rotated 180 to each other, then the 'contour' points would cross through the soma.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, it makes sense to create a new soma type. I will follow the approach of morphologySWC.cpp but in general the current soma type detection has a problem #314.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hold off for now; I'm not sure there's a clean solution, and I think we need to ask around first.

@mgeplf mgeplf added the WIP Work in Progress label May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants