-
Notifications
You must be signed in to change notification settings - Fork 24
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
base: master
Are you sure you want to change the base?
Conversation
2eab8f1
to
dd31a9d
Compare
283d466
to
bd570a7
Compare
if (!nb_.soma()->points().empty()) | ||
throw SomaError(err_.ERROR_SOMA_ALREADY_DEFINED(lex_.line_num())); | ||
nb_.soma()->properties() = properties; | ||
const auto& soma = nb_.soma(); |
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 a new SomaType
is required for a stack, so it can be distinguished from a simple contour?
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 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.
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.
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.
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.
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.
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.
Hold off for now; I'm not sure there's a clean solution, and I think we need to ask around first.
bd570a7
to
ee494c9
Compare
Successor of #39, solution to #300, depends on #294 to update documentation after that.