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

Move NeuroM morphology specs to MorphIO #313

Merged
merged 8 commits into from
May 26, 2021
Merged

Conversation

asanin-epfl
Copy link
Contributor

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

Not finished. Still need to make two svg images.

Move specs about Point, Segment, Section, Neurite from NeuroM to MorphIO.

Solution to #312

@asanin-epfl asanin-epfl added the WIP Work in Progress label May 12, 2021
@asanin-epfl asanin-epfl self-assigned this May 12, 2021
@asanin-epfl asanin-epfl changed the title Move in NeuroM specs Move NeuroM specs May 14, 2021
@asanin-epfl asanin-epfl removed the WIP Work in Progress label May 14, 2021
@asanin-epfl asanin-epfl removed the request for review from mgeplf May 17, 2021 08:38
-------

A section is a series of one or more segments. Each section has a type associated with it. The type shows what part of
the neuron a section represents. The type can be the axon, the soma, and so on.

Choose a reason for hiding this comment

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

could we put here the real list of sectiontypes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

3. forking point, forking point
4. starting point, end point

Starting point is the first point of a section without parent. Forking point is the last point of a section that has

Choose a reason for hiding this comment

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

I would move this paragraph before the bullet point, so we know what the different points represent before we read it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitions go immediately after their usage. Putting them before, breaks the narration.

Neurite
-------

A neurite is essentially a tree of sections(`Section`_). The tree structure implies the following:

Choose a reason for hiding this comment

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

Suggested change
A neurite is essentially a tree of sections(`Section`_). The tree structure implies the following:
A neurite is a tree of sections(`Section`_). The tree structure implies the following:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. It is funny that I took the phrase from NeuroM docs.

the file. In the NRN simulator, the soma which is considered as a section (contrary to MorphIO)
is placed first and then neurites are sorted according to their type.

The final order is the following:

Choose a reason for hiding this comment

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

final in what sense? In neuron ordering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

In MorphIO each section is identified by an ID. By default, the section IDs will correspond to
the order of section appearance while performing a depth-first traversal on every neurites. The
neurite order is the order of appearance in the file. Alternatively, the NRN simulator way of
ordering section can be used by specifying the flag ``morphio::Option::NRN_ID`` when opening

Choose a reason for hiding this comment

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

I use this: options=morphio.Option.nrn_order, is it the same as NRN_ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NRN_ID is a typo. Fixing this. Thank you.

***********************************
Prior to version 3.0.0, when a section had a single child section (aka unifurcation), the child section would be merged
with its parent when reading or writing the file. Since version 3.0.0, merging does not happen when reading. Yet
writing of such sections is not allowed.

Choose a reason for hiding this comment

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

could you point to remove_unifurcation method? If you try to save, what happens, this is called? I'm not sure I tried to save with unifurcations, but I find this weird that we cannot save these as they are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@asanin-epfl asanin-epfl requested a review from arnaudon May 21, 2021 09:18
@asanin-epfl
Copy link
Contributor Author

asanin-epfl commented May 26, 2021

I would still need @tomdele or @mgeplf approval here for merging.

@tomdele
Copy link
Contributor

tomdele commented May 26, 2021

I am checking now

Section ordering
****************
In MorphIO each section is identified by an ID. By default, the section IDs will correspond to
the order of section appearance while performing a depth-first traversal on every neurites. The
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure we a have a stability guaranty with DFS. Isn't the DFS a consequence of the file formats (specially for swc or ascii) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is DFS? Depth-first-...? I agree that this statement is wrong. I bluntly copied it from NeuroM. I need to edit it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Depth-first-search sorry

Copy link
Contributor

@tomdele tomdele May 26, 2021

Choose a reason for hiding this comment

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

Because it depends on the ordering of the neighbors and so it depends on the graph / tree underlying implementation and/or the nodes / edges insertion.
Most extreme case: if you are using something like a adjacency sets (with unordered set) you cannot guaranty a stable iterator on the tree/graph.

Copy link
Contributor Author

@asanin-epfl asanin-epfl May 26, 2021

Choose a reason for hiding this comment

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

Can you please help me with clarification on this. I'm lost in the sources of MorphIO.

Should I just delete this paragraph on section ordering?

Copy link
Contributor

Choose a reason for hiding this comment

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

So this is tricky ...
The section ids are format dependent.

  • For the swc this is indeed a DFS and the sections ids are ordered using the placement of the sample inside the file. So the first neighbor is the first sample found in the file (no matter the sample id).
  • For the ascii same DFS and ordered via the placement inside the file of each sections.
  • For the h5 it depends on the section ordering inside the file. The sections exist already we just read them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, I'm putting this into docs? Would it be ok for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

lol actually after thinking about it and having a better understanding, I think the doc is correct...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

XD, should I put something like "verified by Tom" among the lines?

Copy link
Contributor

Choose a reason for hiding this comment

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

"TomDele seal of approval".

Copy link
Contributor

@tomdele tomdele left a comment

Choose a reason for hiding this comment

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

ok after checking I think this is quite ok !

Thanks for this @asanin-epfl

@asanin-epfl
Copy link
Contributor Author

Thank you for the thorough review!

@asanin-epfl asanin-epfl changed the title Move NeuroM specs Move NeuroM morphology specs to MorphIO May 26, 2021
@asanin-epfl asanin-epfl merged commit 8879b80 into master May 26, 2021
@asanin-epfl asanin-epfl deleted the specs-from-neurom branch May 26, 2021 13:54
@tomdele
Copy link
Contributor

tomdele commented May 26, 2021

Thank you for the thorough review!

That was harder than expected for me :D

mgeplf pushed a commit that referenced this pull request Aug 10, 2021
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.

4 participants