Refactor dendrogram #780

asanin-epfl · 2019-11-08T08:26:05Z

Refactored dendrogram in order to add synapses to it later. Will be a separate PR.

mgeplf

My (fast) first pass looks good, but I need to read it more carefully. Can you post an example of the output?

neurom/view/_dendrogram.py

mgeplf · 2019-11-08T09:07:49Z

neurom/view/_dendrogram.py

+
+        segments = neurom_section.points
+        segment_lengths = np.linalg.norm(
+            np.subtract(segments[:-1, COLS.XYZ], segments[1:, COLS.XYZ]), axis=1)


Suggested change

np.subtract(segments[:-1, COLS.XYZ], segments[1:, COLS.XYZ]), axis=1)

np.diff(segments[:COLS.XYZ], axis=0)

wonder if we can use the segment_lengths statistic?

Do you mean to save it as a property? this.segments_lengths = segment_lengths? Currently I don't see any usage for it later.

I mean use the function that does this already neurom/fst/_neuritefunc.py, but called using get() instead of directly

Ah, ok. I will do the changes after all suggestions have been made.

Unfortunately I couldn't reuse segments_lengths function because it is for neurites. I have sections. Please look neurom/fst/_neuritefunc.py#L204.

neurom/view/_dendrogram.py

asanin-epfl · 2019-11-08T09:36:39Z

Example outputs. You can see that dendrogram became wider. It's because I've added a horizontal padding. Probably I should remove it for backward compatibility. Also I put the leftmost section at (0, 0). This way no dendrogram is on negative X axis. This change I would insist to keep.

Before refactor:

After refactor:

mgeplf · 2019-11-08T09:38:10Z

Cool, looks great.

Probably I should remove it for backward compatibility.
I think it's ok - we try and maintain backwards compat, but for diagrams it doesn't have to be exact, imo.

asanin-epfl · 2019-11-08T09:38:33Z

Example output for the biological cell.

Before refactor:

After refactor:

neurom/view/_dendrogram.py

mgeplf · 2019-11-08T12:12:05Z

neurom/view/_dendrogram.py

+        if dendrogram_root is None:
+            dendrogram_root = self
+            dendrogram_root.processed_section_ids = []
+        if neurom_section.id in dendrogram_root.processed_section_ids:


please use an assert for these kind of checks.

Also, is this necessary? I'm not sure how a cycle would happen - without the check, the dendrogram_root param wouldn't be necessary, and I think the logic would look neater?

In the previous version we had sys.setrecursionlimit(max_depth). I thought to replace this recursion limit with the cycles check. If the recursion limit didn't target cycles validation then I will remove my cycles check.

This is the only major issue left to discuss. I would be happy to remove the cycles check but it would be nice to have a guarantee that we can't receive a cycled Neurite tree. Otherwise we need this check. I don't know where else to put it because we need this check per dendrogram tree instance. So each dendrogram node needs an access to the dendrogram tree instance. dendrogram_root is this tree instance.

@wizmer Do we check that the loaded morphology does is not cycled? By the loaded morphology I mean neurom.load_neuron('morph.swc'). I don't see that we do it but I might overlooked it.

For the structure of the file disallows cycles. For SWC, we don't support non-connected components, so I think that's where it wouldn't load properly. I can't remember for h5.

I don't think the sys.setrecursionlimit(max_depth) was for validation, but probably because without it it would crash for neurites with too many descendents.

I just tried on SWC with the following file and NeuroM did not complain although it should have, scary:

1 1 0 0 0 1. -1 2 3 0 0 0 1. 1 3 3 0 5 0 1. 2 4 3 -5 5 0 0. 3 5 3 6 5 0 0. 3 6 2 0 0 0 1. 6 # <-- cyclic point 7 2 0 -4 0 1. 6 8 2 6 -4 0 0. 7 9 2 -5 -4 0 0. 7

The mut_morphio branch already raises on this file so I don't know if it's worth an implementation.

mgeplf · 2019-11-08T12:19:15Z

neurom/view/_dendrogram.py

+    if isinstance(neuron, Neurite):
+        return Dendrogram(neuron.root_node)
+    SomaSection = namedtuple('NeuromSection', ['id', 'type', 'children', 'points'])
+    soma_section = SomaSection(


is there a way to do this with having a 'fake' soma section? I'm thinking that Dendrogram() could check the type, and then create the 'fake' data inline, instead of it being bundled into an object here, and basically decomposed in the __init__

neurom/view/_dendrogram.py

neurom/view/view.py

wizmer

Re-run the notebook in tutorial/neurom_tutorial.ipynb so that it displays the new-style dendrogram.

wizmer · 2019-11-12T09:37:10Z

neurom/view/dendrogram.py

+            )
+        if dendrogram_root is None:
+            dendrogram_root = self
+            dendrogram_root.processed_section_ids = []


Suggested change

dendrogram_root.processed_section_ids = []

self.processed_section_ids = []

to emphasize on the fact that the processed_section_ids you're setting is the one of the current object.

Here I would keep dendrogram to emphasize that there is a tree head and self is this head now.

I dropped the cycles check because it is implemented in mut_morphio branch and the NeuroM core is a more appropriate place for this check.

neurom/view/dendrogram.py

wizmer · 2019-11-12T10:26:29Z

neurom/view/_dendrogram.py

+        if dendrogram_root is None:
+            dendrogram_root = self
+            dendrogram_root.processed_section_ids = []
+        if neurom_section.id in dendrogram_root.processed_section_ids:


I don't think the sys.setrecursionlimit(max_depth) was for validation, but probably because without it it would crash for neurites with too many descendents.

I just tried on SWC with the following file and NeuroM did not complain although it should have, scary:

1 1 0 0 0 1. -1 2 3 0 0 0 1. 1 3 3 0 5 0 1. 2 4 3 -5 5 0 0. 3 5 3 6 5 0 0. 3 6 2 0 0 0 1. 6 # <-- cyclic point 7 2 0 -4 0 1. 6 8 2 6 -4 0 0. 7 9 2 -5 -4 0 0. 7

The mut_morphio branch already raises on this file so I don't know if it's worth an implementation.

neurom/view/dendrogram.py

asanin-epfl · 2019-11-12T13:22:58Z

I removed the cycles check and fake SomaSection. @mgeplf and @wizmer please verify.

asanin-epfl · 2019-11-12T13:32:33Z

Re-run the notebook in tutorial/neurom_tutorial.ipynb so that it displays the new-style dendrogram.

@wizmer I ran it and it worked fine. If you see a problem please say it directly.

wizmer · 2019-11-12T13:35:09Z

It works, but the picture of the dendrogram cell #6 is the one from the old algo, so I just re-run this cell and update the notebook.

asanin-epfl · 2019-11-12T13:40:14Z

It works, but the picture of the dendrogram cell #6 is the one from the old algo, so I just re-run this cell and update the notebook.

Thank you for the details. Fixed it. I reran the first two cells for imports + #6 cell.

asanin-epfl added 2 commits November 7, 2019 18:39

Refactor Dendrogram

29ffb02

py27 compatibility

b20df41

asanin-epfl requested review from mgeplf and eleftherioszisis November 8, 2019 08:45

asanin-epfl self-assigned this Nov 8, 2019

mgeplf reviewed Nov 8, 2019

View reviewed changes

fix review comments

1c10023

wizmer reviewed Nov 12, 2019

View reviewed changes

neurom/view/dendrogram.py Outdated Show resolved Hide resolved

asanin-epfl added 2 commits November 12, 2019 14:02

drop cycles check

0ad8b4f

remove fake SomaSection

a6eee2a

update neurom_tutorial with the picture of the new dendrogram

fd4ffaf

wizmer approved these changes Nov 12, 2019

View reviewed changes

asanin-epfl merged commit cb77caa into master Nov 12, 2019

asanin-epfl deleted the refactor-dendrogram branch November 12, 2019 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor dendrogram #780

Refactor dendrogram #780

asanin-epfl commented Nov 8, 2019

mgeplf left a comment

mgeplf Nov 8, 2019

mgeplf Nov 8, 2019

asanin-epfl Nov 8, 2019

mgeplf Nov 8, 2019

asanin-epfl Nov 8, 2019

asanin-epfl Nov 11, 2019

asanin-epfl commented Nov 8, 2019

mgeplf commented Nov 8, 2019

asanin-epfl commented Nov 8, 2019

mgeplf Nov 8, 2019

asanin-epfl Nov 11, 2019

asanin-epfl Nov 11, 2019 •

edited

Loading

asanin-epfl Nov 11, 2019

mgeplf Nov 12, 2019

wizmer Nov 12, 2019

mgeplf Nov 8, 2019

wizmer left a comment

wizmer Nov 12, 2019

asanin-epfl Nov 12, 2019

asanin-epfl Nov 12, 2019

wizmer Nov 12, 2019

asanin-epfl commented Nov 12, 2019

asanin-epfl commented Nov 12, 2019

wizmer commented Nov 12, 2019

asanin-epfl commented Nov 12, 2019

	np.subtract(segments[:-1, COLS.XYZ], segments[1:, COLS.XYZ]), axis=1)
	np.diff(segments[:COLS.XYZ], axis=0)

	dendrogram_root.processed_section_ids = []
	self.processed_section_ids = []

Refactor dendrogram #780

Refactor dendrogram #780

Conversation

asanin-epfl commented Nov 8, 2019

mgeplf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asanin-epfl commented Nov 8, 2019

Before refactor:

After refactor:

mgeplf commented Nov 8, 2019

asanin-epfl commented Nov 8, 2019

Before refactor:

After refactor:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asanin-epfl Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wizmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asanin-epfl commented Nov 12, 2019

asanin-epfl commented Nov 12, 2019

wizmer commented Nov 12, 2019

asanin-epfl commented Nov 12, 2019

asanin-epfl Nov 11, 2019 •

edited

Loading