Added new brain::Section class and related methods in brain::Morphology.

[hernando]

The new class provides access to morphological data on a per
section basis. From a section it's possible to query the parent and
children as well as information for its sample points. The new
class is very lightweight so it can be created on-the-fly by
brain::Morphology and doesn't need to be stored anywhere.

[hernando]

Something I'd like to discuss is whether we should consider that the soma is also a Section or not.

[bilgili]

for( const SectionType type: *types ) ? or BOOST_FOREACH ?

[hernando]

None is possible (C++11 even less, because we can't use it in Brion).
Note that line 85 needs the index.

[eile]

+600 loc... ugh.

There are plenty of new functions in morphology.cpp which do not appear in morphology.h? I got lost in the naming and wanted to read doxygen first - unless I missed something, can you make this PR more understandable, or I will -1 it on grounds of fighting potential SDK dragons. ;)

[hernando]

I can do as you request by moving the detail::Morphology into it's separate .h/.cpp files. Originally detail::Morphology was Morphology::Impl, but the implementation of Section needs to access the Morphology internals so I had to move it to a public class (I preferred that to making it friend). I kept evertyhing in one file being conscious that someone could complain, but there wasn't any example of this pattern I could use as a reference. If I do this change morphology.cpp will be split into 3 files.

On the other hand -1 without actual facts on what's wrong, only on the assumption that something might be wrong is not fair. That would be an authority veto, which is something I don't agree with.

[eile]

the implementation of Section needs to access the Morphology internals

That's what I missed. Weird pattern. In that case, these methods should be considered public and documented. For example, I have no clue what getSectionSamples does.

[eile]

-1 would be on grounds that I don't understand, not on authority (which is wrong). Apologies of putting you on the spot, but I want to finally understand this stuff and to avoid complexity where possible.

[eile]

the implementation of Section needs to access the Morphology internals

Not having understood the code: maybe the relevant methods should be part of the Morphology API?

[eile]

brain::Morphology?

[hernando]

yes

[dnachbaur]

I'm not comfortable with the impl sharing between morphology and section. Reminds too much what the SDK did wrong in my opinion. The section class is public, but is not allowed to create it standalone as it always needs to be created by morphology. Seems a step backward wrt RAII and unit tests.

Why not having the section relevant functions part of morphology?

[hernando]

To me, the design is much more OO with the existence of a Section class. Compare the following:

float dendriticLengh = 0;
for( auto section : morphology.getSections({ SECTION_DENDRITE, SECTION_APICAL }))
    dendriticLength += section.getLength();

and

float dendriticLengh = 0;
for( auto id : morphology.getSectionIDs({ SECTION_DENDRITE, SECTION_APICAL }))
     dendriticLength += morphology.getSectionLength( id );

The second one looks like doing OO with C in my opinion. And since the sections keep track of the morphology safely internally, you don't need to pass the morphology reference to code operating on sections.

Your claim about being a step backward in relation to RAII and unit tests should be more concrete.
I don't understand what's the weakness that you see in this design related to RAII. Since the class doesn't have a public constructor for me there's no statement to do about RAII, it doesn't apply.
It's true that the Section class cannot be tested separately, but saying that's wrong is based on the assumption that there's a one-to-one mapping between units and classes. That's generally true, but if it had to be always the case, then why aren't they called class tests? On the other hand, it you consider that the unit is the Morphology, Section pair (which is still a small unit), then it makes sense.

Last but not least. Adding all the methods to the morphology class makes the interface look more bloated and the function names longer.

I need more solid argumentation that this is an anti-pattern or a consensus against it to revert it.

Would it make you feel more comfortable if I declare Section as a nested class of Morphology?

[rdumusc]

conform -> compose or form

[hernando]

done

[hernando]

@tribal-tec, since you didn't answer I think I didn't convince you. Regardless of the implementation, the proposed API is easy to test in my opinion (and there are tests), and I'm trying to find a reason why the design is flawed in terms of RAII, but I don't find it. Why do you think it's better to have all the functions in the morphology class (and don't focus on the implementation for the moment)?

[dnachbaur]

I haven't enough time yet to reply. I have a more philosophical problem where we define an API for something we don't really own (but use of course), w/o involving people from other teams and/or looking what was done in that regard on their end (bluepy, neurom, ...). I'm afraid it will converge to an 'SDK-light'.

The section class right now is wrapper on top of data that is owned by Morphology. That ownership is shared across any number of sections that may have been returned which can be implemented in a safe way, but still relies on the presence of a morphology object (worst case: cf segment points this-pointer iteration because data is stored in a vector!!). If you just look on the section class, it cannot be used by itself w/o a morphology (which semantically might make sense), but then RAII is ill-formed to me as there is no resource owned or managed by that section object.

Your outlined examples wrt computing the dendrite length is outlining another thing: If a user wants to know the length of the dendrite, he/she has to compute it rather than just using a getDendriteLength() function. Of course, in the extreme that means there is >100 functions for all the different use cases, although I believe for the basic things we do with morphologies it should be reasonable small. For a high(er)-level API I would except such things rather than a convenience wrapper on top of the loader class which does not provide more real functionality. This is potentially a discussion points with other users what makes sense to them.

Last thing regarding the soma. I would not model it as section given the special handling it imposes on the section class. For neuron/cell type of morphology a soma/cell body is always present (correct me if I'm wrong), so it should be defined part of the morphology.

[hernando]

Your concern about ending up with an SDK-light is reasonable, but don't be biased by assuming that everything inside the SDK is wrong, because some ideas are reusable.
Given my lack of knowledge of client code using other tools (NeuroN, BluePy), I've designed thinking of the minimal feature set needed by BRayns and RTNeuron (and not all in the case of RTNeuron), drawing ideas from BBPSDK and heavily simplifying it (no Segment, no Cross_Section, no Segments or Sections). It think the end result is neater and even more compact. It may even be more efficient because it uses fewer pointers and indirections, but I don't have hard data.

I took a quick look at NeuroM and what I've found there is even higher level (and less clear IMO, with funny things like SomaA, SomaB and SomaC as class names, but I'm also biased), I didn't find the notion of section ID for example. In BluePy they are using the BBPSDK morphology and reader and then doing some post-processing to provide bounding boxes and a transposed view on the data (the documentation is not sufficiently clear but reading the code there a Sections class that converts each section into a tuple of numpy arrays. In any case, what I see on those APIs is something that could build on top of this. Having a Section class becomes a matter of taste, I don't see the design risk.

One argument in favor of having the Section class is that it allows better information hiding. With the Section object you can forget completely about IDs, which is a detail of the data model. In the end some algorithms and data structures need to use IDs for compactness and referencing (e.g. synapses), so it's a matter of a trade-of.
But be aware that obeying users without questioning may be dangerous. According to statements I've heard many times, many end users simply want to have data file specifications and write code to read the data directly their own way, and that's wrong in so many aspects that I'm not going to extend further.

As for the comment you've made about getDendriteLength I think you've answered yourself. I don't want to take the risk of API inflation, so I haven't added anything for which I don't see an immediate use case. For example, providing the bounding volume of the morphology seems to be a very good candidate for addition.
On the other hand, I'm afraid that with NeuroM we are going to be the only consumers of this API in the end.

Regarding RAII I think you're making a logical mistake when deriving your conclusion (affirming the consequent). RAII does not imply IIRA. Of course depending on what's a resource for you, you may argue than initialization always implies resource acquisition (meaning by acquisition something more general than allocation, e.g. getting a handler to an already allocated resource). Following your line of thought you could also conclude that iterators are wrong because they don't allocate anything and are just wrappers to access the items contained by a collection.

Do we agree on having a separate class for the soma then?
If that's the case there's a small issue with Morphology::getSection(). When the requested section is the soma this method will have to throw. The only way to avoid this problem is not assigning an ID to the soma, but that's not possible with our data model.

[dnachbaur]

So you'd rather take the risk of class inflation :p At this point it seems we're just discussing code anyways, so I'll play along.

For the soma, to follow your design it probably is a separate class. But it's catch-22 for the mentioned problems with getSection() with soma ID vs Section class functions that are semantically wrong/non-existent for soma sections.

[hernando]

ping

[eile]

All: please vote:

Option 1: As above, except s/cell/neuron/g
Option 2:

morphology.h
morphology/section.h
morphology/soma.h

Option 1 rationale:

All functionalities wrt morphologies contained in a single namespace. Adding glial morphology would not collide.

Option 2 rationale:

This option mimics nested classes cleanly using namespaces.

Symmetry with brion::Morphology and in naming: for section, it's a morphology::Section. Maybe morphology should be renamed neuron/morphology here and in brion:: (tbd when glia is introduced: aka defer Option 1).

[jafyvilla]

I like option 1 better: neuron/*.h

[bilgili]

I don't know much about glia to answer the question. Are morphology and gliamorphology different in terns of behaviours ? Sections etc. Or there will be a enumaration in morphology to state the type ?

[hernando]

As far as I know, they are different enough to make it difficult to provide base classes for both. Besides that I don't know if there's any use case that requires the base class.

[rdumusc]

Very hard to say.
As it is now, I have a slight preference for the option 1, as having a "neuron" namespace makes the meaning of the classes very clear: brain::neuron::Morphology, brain::neuron::Section, brain::neuron::Soma.
If Soma is not a nested class of Morphology, because the implementation would be ugly, then I have doubts about replacing the concept with an extra namespace.
With option 2, we will still have the name collision with a future glia::Morphology, unless it is combined with option 1 later as you also suggest. But then the hierarchy becomes quite long, and I am not sure if the "morphology" namespace brings much additional information in a brain::neuron::morphology::Soma.
(The last unknown is how a future glia morphology would be implemented. I am assuming here that they will be different enough and there will not be the use for a class hierarchy between the two...)

[hernando]

There wouldn't be a brain::neuron::morphology::Soma in any case. It would be brain::neuron::Soma even for option 2 if it's later reworked to consider brain::glia (as Stefan says, option 2 is to defer option 1 until needed and be as consistent with brion as possible for the moment).

[bilgili]

I am finding it difficult to vote because of the unknowns about different cells, different morphologies. If morphology is skeleton it should not be so much different than the glia morphology. If they are really different they should have different folders and namespaces. If they look a lot like, there would be only type informations and then afterwards we don't even need a cell directory ( my previous flat model idea is still valid ).

Finally I am voting for "1", but it is like a coin flip where I like the HEADs a bit better and sending a "reiki" energy towards ! :)

[hernando]

Glial morphologies have elliptical cross sections. That alone is a big change. Then the section types are completely different also. The most correct thing would even be to create a new section type. Even more, I don't even know if a single class would be enough to represent the morphologies of all types of glial cells.

[bilgili]

To play the devils advocate, circles are special cases of elipses :) "a=b". If we have a section that is common between neuron and glia that should be fine.

[hernando]

Except that ellipses require at least 2 more floats to be fully defined (focal distance and angle), which is a waste for millions of points if what you actually have is circles. But let's not discuss this yet.

[dnachbaur]

missing != operator

[hernando]

All comments have been addressed, I don't know why Github is not obsoleting them.