-
Notifications
You must be signed in to change notification settings - Fork 24
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.