Feat/export swc:- This aims to add a file which exports SWC file into neuroml. #407

AdityaBITMESRA · 2024-07-18T11:43:27Z

This draft PR aims to add a file which exports SWC into neuroml for visualisation

It is not complete mainly around the soma representation part.
Will be making it more efficient and reliable so that it can cover all swc cases.

AdityaBITMESRA · 2024-07-29T08:06:04Z

Hi @sanjayankur31 I have refactored the code. Can you give it a look if time permits?

sanjayankur31 · 2024-07-29T09:46:19Z

Cool, will have a look at it today or tomorrow hopefully.

sanjayankur31 · 2024-07-29T10:28:06Z

Did we add any unit tests for the writer @AdityaBITMESRA ? (Not seeing them in this PR)

AdityaBITMESRA · 2024-07-29T11:43:34Z

No, I will add them soon

AdityaBITMESRA · 2024-07-29T11:51:43Z

I wanted to ask how should I structure my test file like I should build a mock swc data for all the cases?

sanjayankur31 · 2024-07-29T12:15:43Z

I wanted to ask how should I structure my test file like I should build a mock swc data for all the cases?

Yeh, you can build mock data, or you can even copy the case files from the CVApp if you want, or you can only paste their contents into a Python string in the test? Whatever you think works best.

…to feat/ExportSWC

sanjayankur31

Looks very good. Getting there. A few notes/queries/suggestions there.

pyneuroml/swc/ExportNML.py

sanjayankur31 · 2024-07-30T11:10:39Z

pyneuroml/swc/ExportNML.py

+
+
+class NeuroMLWriter:
+    def __init__(self, swc_graph: "SWCGraph"):


does SWCGraph need to be in quotes here?

sanjayankur31 · 2024-07-30T11:12:54Z

pyneuroml/swc/ExportNML.py

+        start_point = self.find_start_point()
+
+        print(f"Cell name: {cell_name}")
+        print(f"Start point: {start_point}")


if you're using these for debugging, use logging so that you can use logger.debug and so on. Only stuff that is definitely needed to be printed to the user should use print.

pyneuroml/swc/ExportNML.py

sanjayankur31 · 2024-07-30T11:16:26Z

pyneuroml/swc/ExportNML.py

+        :param this_point: The current SWCNode.
+        :param parent_point: The parent SWCNode.
+        :param cable_id: The ID of the current cable.
+        :param version: The NeuroML version being used.


Sorry, we should've mentioned this---NeuroMLv1 is deprecated and not to be used at all. So, no need to have special cases supporting it. We only export to NeuroMLv2.

sanjayankur31 · 2024-07-30T11:47:26Z

pyneuroml/swc/ExportNML.py

+        self.next_segment_id += 1
+        self.point_indices_vs_seg_ids[this_point.id] = seg_id
+
+        segment = [f'      <segment id="{seg_id}" name="Seg_{seg_id}"']


Now that this is all neuroml specific, is there a reason we're not creating objects using the libneuroml API? (Segment and SegmentGroup)? It's usually better than creating xml by hand.

Can I get some more ideas on how to proceed with this? I am bit confused about using the libneuroml API.
@sanjayankur31

Sure, see this example for a start:

https://docs.neuroml.org/Userdocs/MultiCompartmentOLMexample.html#declaring-the-cell

When users use NeuroML, they don't write XML---that's tedious and error prone. Instead, they use the libNeuroML API:

cell = neuroml.Cell(...) cell.add_segment(...) ..

it's basically a Python object model of the NeuroML schema, so a Cell contains Morphology, which contains Segments and SegmentGroups.

That's a better way of generating the NeuroML than writing XML by hand.

Let me know if this isn't clear and we can have a chat where I can explain it to you more.

Thankyou I understand it now...will try to implement this

Do take a look at the various "utility functions" in the Cell class, they should help with lots of things (hopefully):

https://libneuroml.readthedocs.io/en/latest/userdocs/coreclasses.html#cell

I've just remembered: if you just add all the segments with their correct parents to the cell, you should be able to use the create_unbranched_segment_group_branches to automatically create all the unbranched segment groups. It creates the adjacency matrix and then traverses the whole cell detecting branches:

https://libneuroml.readthedocs.io/en/latest/userdocs/coreclasses.html#neuroml.nml.nml.Cell.create_unbranched_segment_group_branches

sanjayankur31 · 2024-07-30T11:51:50Z

pyneuroml/swc/ExportNML.py

+            ],
+        }
+        groups.extend(
+            type_groups.get(p.type, [f"SWC_group_{p.type}", "dendrite_group"])


what are we doing here? if a group for the type is not found, we're using SWC_group_{type} and dendrite_group? Not sure if this is correct. If it doesn't belong to the standard types, it shouldn't be valid SWC 🤔

sanjayankur31 · 2024-07-30T11:52:43Z

pyneuroml/swc/ExportNML.py

+                self.group_content.append(f"        <meta:group>{group}</meta:group>")
+            self.group_content.append("      </cable>")
+        else:
+            for group in self.get_groups_for_type(this_point):


if this is the first point of a new branch, it should also create a new segment group.

ALL "cables" also have to have their own segment groups, in addition to the standard soma/dendrite etc groups. Is this being done?

AdityaBITMESRA · 2024-07-30T12:03:31Z

Thankyou for all the comments @sanjayankur31 I will update after addressing the comments

AdityaBITMESRA · 2024-08-05T07:38:16Z

Hi @sanjayankur31 I have tried to address all the comments so can you have a look if time permits...it is still not complete but I wanted to know if I am going right or not

sanjayankur31

It's looking good and you're on the right track. Well done. 👍

Dropped a few comments about things I wasn't completely clear on.

sanjayankur31 · 2024-08-05T12:24:48Z

pyneuroml/swc/ExportNML.py

+        Initialize the NeuroMLWriter.
+
+        :param swc_graph: The graph representation of the SWC file.
+        :type swc_graph: Any  # Replace with the actual type of swc_graph if known


Should probably be your SWCGraph class?

sanjayankur31 · 2024-08-05T12:25:10Z

pyneuroml/swc/ExportNML.py

+
+
+class NeuroMLWriter:
+    def __init__(self, swc_graph):


Please also remember to add the type hint here.

sanjayankur31 · 2024-08-05T12:26:07Z

pyneuroml/swc/ExportNML.py

+        logger.info("Initializing NeuroMLWriter")
+        self.swc_graph = swc_graph
+        self.points: List[SWCNode] = swc_graph.nodes
+        self.section_types: List[str] = [


are these not already defined in the other module? Can we re-use that so that the same bits aren't defined in two different locations?

sanjayankur31 · 2024-08-05T12:31:26Z

pyneuroml/swc/ExportNML.py

+        self.next_segment_id += 1
+
+        segment = Segment(id=seg_id, name=f"Seg_{seg_id}")
+        segment.type = this_point.type


neuroml.Segment does not have a type attribute

sanjayankur31 · 2024-08-05T12:47:50Z

pyneuroml/swc/ExportNML.py

+            diameter=2 * this_point.radius,
+        )
+
+        self.cell.morphology.segments.append(segment)


why not also place the segment in the default group here when it has been created? All segments should go into the all group, and then here since you already check what the segment type is, you can also at least place it in the soma/dendrite/axon group?

Otherwise you're iterating over all segments again in the create_segment_group

sanjayankur31 · 2024-08-05T12:48:35Z

pyneuroml/swc/ExportNML.py

+        """
+        logger.info("Creating segment groups")
+
+        def get_groups_for_type(segment_type: int) -> List[str]:


see above comment, can this be done while creating the segment so it's done in the same pass, instead of doing a second pass over all the segments? (if you need information about all segments to do this, then a second pass is required, that's OK too).

sanjayankur31 · 2024-08-05T12:54:45Z

pyneuroml/swc/ExportNML.py

+
+        if this_point.type == SWCNode.SOMA:
+            self.handle_soma(this_point, parent_point, cable_id, version, new_cell)
+        elif this_point.type != SWCNode.SOMA and parent_point.type == SWCNode.SOMA:


Hrm, I'm unclear on his---the first point of a dendrite will have a soma point as parent, no?

This is tricky to understand. Maybe add a comment before the conditional explaining the three cases so that others reading the code are clear on what's happening here?

Thankyou for pointing this out...I will add comments to make it more clear

sanjayankur31 · 2024-08-05T13:00:27Z

pyneuroml/swc/ExportNML.py

+        logger.info("Creating Cell object")
+        cell_name = self.get_cell_name()
+
+        self.cell = Cell(id=cell_name)


I'd maybe use the component_factory here:

from neuroml.utils import component_factory cell = component_factory("Cell", id="cell_name", notes="something")

(the advantage of the component factory is that it validates things as we go)

sanjayankur31 · 2024-08-05T13:03:16Z

pyneuroml/swc/ExportNML.py

+
+        logger.info("Segment groups created successfully")
+
+    def print_soma_segments(self) -> None:


is this for debugging?

you can use cell.get_segment_group("soma_group") to get all segments in the segment group.

See help(neuroml.Cell) in a python interpreter to see all the methods of the Cell class.

yes I added this for debugging...will go through the recomendation

Cool, I'd also suggest writing unit tests for testing that each part of your code is doing what you expect it to do :)

AdityaBITMESRA · 2024-08-06T01:46:30Z

I have added a unittest file and will make it more perfect with time

sanjayankur31 · 2024-08-07T07:44:43Z

pyneuroml/swc/ExportNML.py

+import tempfile
+from typing import Dict, List, Optional, Set
+
+import neuroml.writers as writers


This looks good, well done. I think the file should be test_ExportNML.py, just for consistency, though.

…NeuroML into feat/ExportSWC

AdityaBITMESRA · 2024-08-21T13:25:45Z

Hi @sanjayankur31 I have addressed the issues...Can you give it a look?

AdityaBITMESRA · 2024-08-27T09:43:13Z

I was running with this swc file @sanjayankur31

AdityaBITMESRA · 2024-08-27T09:50:25Z

This is the plotted morphology of neuroml generated from the python based converter

This is from the cvapp

AdityaBITMESRA · 2024-08-27T09:56:08Z

I went through the neuroml file in the soma segment 70 the proximal is set to the previous segment distal which is creating the problem.

When I am manually writing the proximal to that of point id 1 which is its parent according to the swc data I am getting the exact morphology like the cvapp

sanjayankur31 · 2024-08-27T10:13:04Z

Cool, I think I know what's going on. I'll look into it soon.

sanjayankur31 · 2024-08-27T15:17:27Z

Working on this here: #421

(This will be automatically merged when that one gets merged, so leaving this open)

AdityaBITMESRA added 3 commits July 18, 2024 17:03

Added ExportNML.py file to export SWC into NeuroML

c24a91b

removed example usage

d61a66b

removed unused variable

8860733

AdityaBITMESRA marked this pull request as draft July 19, 2024 08:09

refactored the whole code

90de9ce

AdityaBITMESRA marked this pull request as ready for review July 29, 2024 08:04

sanjayankur31 self-requested a review July 29, 2024 09:45

sanjayankur31 added the S: ready for review Status: ready for review label Jul 29, 2024

Merge branch 'development' of https://github.com/NeuroML/pyNeuroML in…

d12cd95

…to feat/ExportSWC

sanjayankur31 requested changes Jul 30, 2024

View reviewed changes

AdityaBITMESRA added 4 commits July 31, 2024 14:27

refactored for handling wrong node types

609dfaa

added logger

b845beb

refactored the code with reference to using the neuroml library

6223b02

removed unused imports

7bd56db

sanjayankur31 self-requested a review August 5, 2024 12:24

sanjayankur31 reviewed Aug 5, 2024

View reviewed changes

AdityaBITMESRA added 4 commits August 6, 2024 05:49

added SWCGraph and removed Segment.type

e1886b1

added __init__.py file

2da58d1

added unittest file

c617ef2

removed absolute import

1bd17ce

refactored to add component_factory

2112586

sanjayankur31 reviewed Aug 7, 2024

View reviewed changes

AdityaBITMESRA added 4 commits August 7, 2024 13:52

renmaed the test file

6e1a738

renmaed the test file

ca5c326

renmaed the test file

4dd5006

added comments to provide more clarity

f46f933

sanjayankur31 self-requested a review August 8, 2024 10:23

AdityaBITMESRA and others added 13 commits August 8, 2024 19:30

removed component_factory

9fa84cc

refactored the Export file

3a5a8cc

refactored the code for better segment formation

ab280ac

Delete pyneuroml/swc/dCH-cobalt.CNG.nml

fcca73b

refactore docstrings

38c24b0

Merge branch 'feat/ExportSWC' of https://github.com/AdityaBITMESRA/py…

e7dc36b

…NeuroML into feat/ExportSWC

refactored docstrings

aa40ae5

used relative imports

34642ac

removed the segment duplication error

6d98ec2

some changes

ad92502

changes

5bad25f

changes

89b5fd9

resolved the non-processing of non soma points error

1509d16

AdityaBITMESRA added 3 commits August 21, 2024 20:14

resolved the non-processing of non soma points error

b88b1a3

added detailed docstrings

d1d75f4

refactored multipoint soma handling

7290dc9

sanjayankur31 merged commit 612815d into NeuroML:development Sep 4, 2024
7 checks passed



		class NeuroMLWriter:
		def __init__(self, swc_graph: "SWCGraph"):


		logger.info("Segment groups created successfully")

		def print_soma_segments(self) -> None:

Feat/export swc:- This aims to add a file which exports SWC file into neuroml. #407

Feat/export swc:- This aims to add a file which exports SWC file into neuroml. #407

Conversation

AdityaBITMESRA commented Jul 18, 2024

AdityaBITMESRA commented Jul 29, 2024

sanjayankur31 commented Jul 29, 2024

sanjayankur31 commented Jul 29, 2024

AdityaBITMESRA commented Jul 29, 2024

AdityaBITMESRA commented Jul 29, 2024

sanjayankur31 commented Jul 29, 2024

sanjayankur31 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

AdityaBITMESRA Jul 31, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaBITMESRA commented Jul 30, 2024

AdityaBITMESRA commented Aug 5, 2024

sanjayankur31 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

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

AdityaBITMESRA commented Aug 6, 2024

Choose a reason for hiding this comment

AdityaBITMESRA commented Aug 21, 2024

AdityaBITMESRA commented Aug 27, 2024 • edited Loading

AdityaBITMESRA commented Aug 27, 2024 • edited Loading

AdityaBITMESRA commented Aug 27, 2024

sanjayankur31 commented Aug 27, 2024

sanjayankur31 commented Aug 27, 2024

AdityaBITMESRA Jul 31, 2024 •

edited

Loading

AdityaBITMESRA commented Aug 27, 2024 •

edited

Loading

AdityaBITMESRA commented Aug 27, 2024 •

edited

Loading