[WIP] nmlimport package: importing morphologies from nml file #17

kapilkd13 · 2018-05-23T23:23:51Z

This PR features current progress in implementing nmlimport package that allows importing morphologies from nml file.

kapilkd13 · 2018-05-28T18:28:38Z

Hi @mstimberg I am writing test for load_morphology function. Here I am comparing the final morphology object attributes to pre determined values to assert our function is correct. I am checking attributes like length,distance, coordinates and area. Although other three are providing correct result, area is failing assertion. see line https://github.com/brian-team/brian2tools/pull/17/files#diff-1a36c24e3b7044d2e6d50df875dc2968R63
output that we get:

=================================== FAILURES ===================================
_____________________________ test_load_morphology _____________________________

    def test_load_morphology():
        morphology = load_morphology("samples/sample1.cell.nml")
        assert_equal(morphology.distance,[ 8.5]*um)
        assert_equal(morphology.length,[ 17.]*um)
        assert_equal(morphology.coordinates,[[0.,0.,0.],[0.,17.,0.]]*um)
    
>       assert_equal(morphology.area,[1228.36272755]*um2)
E       AssertionError: 
E       Arrays are not equal
E       
E       (mismatch 100.0%)
E        x: Quantity([  1.228363e-09])
E        y: Quantity([  1.228363e-09])

Can you think on any reason? Can float-point precision be behind it.

mstimberg · 2018-05-28T18:59:56Z

Can you think on any reason? Can float-point precision be behind it.

Yes, it's very likely almost the same value. For these kind of comparisons, rather use assert_allclose instead of assert_equal.

kapilkd13 · 2018-05-29T19:00:03Z

Hi @mstimberg I have added tests and docstring. Please have a look. I haven't updated sphinx doc to auto import docstring as due to some reason sphinx is not working on my pc for this particular project.
make html gives me

sphinx-build: error: unrecognized arguments: /home/kapil/github/brian2tools/brian2tools /home/kapil/github/brian2tools/brian2tools/tests

I suspect it has to do something with the fact that I have pip develop this module instead of building it.

Also, passing file object works with libNeuroML (it is using lxml), although in docs it mention for file location only. I have updated load_morphology accordingly.

mstimberg · 2018-05-31T17:07:00Z

due to some reason sphinx is not working on my pc for this particular project.

Mmm, that's odd, it should work with pip install -e . (that's what I'm using) or python setup.py develop. What happens if you directly run sphinx-build -b html -d ../docs/doctrees . ../docs/html in docs_sphinx? That should be what the makefile does.

I haven't updated sphinx doc to auto import docstring

You don't need to do anything to import the docstrings. The conf.py file automatically runs sphinx-apidoc to create the reference documentation for all files in the package.

Could you convert your docstrings to the numpydoc format? I find it much more readable in the source code and it is what we are using in the rest of brian2 and brian2tools.

kapilkd13 · 2018-06-01T09:33:44Z

Hi @mstimberg I have updated the docstring. I tried the sphinx build command but it also throws same error.
Travis build is failing right now because we need libneuroml for testing but its latest package is not available for conda. Since tests are a part of conda recipe, pip installing libneuroml in travis file after the conda environment is built doesn't work either. We can create and upload libneuroml package for conda and use that. Or we can move tests out of conda recipe and into travis file, would that work? Is there some other way?

kapilkd13 · 2018-06-07T14:08:07Z

@mstimberg I have made conda recipe for libNeuroML, can you have a look and see if it is good to go.
https://github.com/kapilkd13/staged-recipes/blob/libneuroml/recipes/libneuroml/meta.yaml

mstimberg · 2018-06-07T14:59:39Z

Hi @kapilkd13 , this is looking very good! Just some minor things to make it perfect:

Since libneuroml can be used with Python 2 and 3 without changes and is "pure Python", it can be a noarch package (but we should check back with Padraig, setup.py only mentions Python 2 in its classifiers)
The installation should use pip which therefore needs to be a build dependency
In addition to the dev_url, you could also add http://libneuroml.readthedocs.io as the doc_url

kapilkd13 · 2018-06-08T12:09:29Z

@mstimberg I forgot to mention I have developed the test script we were talking about. Its in the last commit here

mstimberg · 2018-06-08T12:10:25Z

No worries, I saw the commit :)

kapilkd13 · 2018-06-10T12:47:54Z

@mstimberg I added function to generate proximal distance from parent and its working fine. But their is another issue, while validating we are checking if the proximal distance of a segment is matching to the distal of its parent but in some files their is fraction along is used and in these cases proximal point do not overlap with distal. This throws validation error. In this file https://github.com/NeuroML/NeuroML2/blob/master/examples/NML2_SimpleMorphology.nml
we are getting

brian2tools.nmlimport.nml.ValidationException: <Segment|3|Spine> and its parent segment <Segment|2|MainDendrite2> are not connected!!

mstimberg · 2018-06-10T12:53:05Z

Hi, this is fine, we don't support these morphologies yet, anyway. I think the best thing would be to raise a NotImplementedError if a segment defines fractionAlong with a value different from 1.

kapilkd13 · 2018-06-11T15:52:00Z

Hi @mstimberg I added runtime dependencies to conda recipe. Should we push it now https://github.com/kapilkd13/staged-recipes/blob/libneuroml/recipes/libneuroml/meta.yaml

mstimberg · 2018-06-11T16:48:12Z

Hi @mstimberg I added runtime dependencies to conda recipe. Should we push it now https://github.com/kapilkd13/staged-recipes/blob/libneuroml/recipes/libneuroml/meta.yaml

It is looking good, but some of the tests fail for me when building/installing/testing the package locally. It seems to be an issue with the jsonpickle library, if I install the 0.7.2. version from pip (unfortunately it's not packaged for conda-forge), the test runs fine. Could you maybe have a quick look at the bug, maybe it's something that is easily fixable in libneuroml (possibly via a try...except that tries something that works for the latest versions and falls back to something that works for earlier versions of jsonpickle)?

Oh, and I think you can remove vellamike from the maintainers, according to Padraig he left academia so I guess he's not particularly interested in maintaining the package.

mstimberg · 2018-06-11T16:48:57Z

brian2tools/nmlimport/nml.py

@@ -31,7 +31,7 @@ def validate_morphology(segments):
        for segment in segments:

            if segment.parent is not None:
-                if segment.parent.fraction_along != 1:
+                if segment.parent.fraction_along not in [0,1]:


I don't think we correctly support fraction_along=0, it would mean to connect to the proximal end of a segment.

Currently I was checking if it is connected proximally here I guess we should remove it then :)

kapilkd13 · 2018-06-11T19:04:34Z

Hi @mstimberg how are you testing conda recipe locally, currently I am using conda-build . to build recipe after adding

  commands:
    -  py.test --pyargs neuroml
  requires:
    - pytest

to meta.yml file. On running I am getting errors like

  def read_neuroml2_file(nml2_file_name, include_includes=False, verbose=False,
                           already_included=[], print_method=print_, optimized=False):
    
        print_method("Loading NeuroML2 file: %s" % nml2_file_name, verbose)
    
        if not os.path.isfile(nml2_file_name):
            print_method("Unable to find file: %s!" % nml2_file_name, True)
>           sys.exit()
E           SystemExit

../_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehol/lib/python3.6/site-packages/neuroml/loaders.py:235: SystemExit

Is this the right approach? Also how did you pip installed specific version of jsonpickle with conda recipe. is pip allowed in meta file?

mstimberg · 2018-06-11T19:20:11Z

Sorry, I should have been clearer about this. I don't do any change to the meta.yaml file (I don't think that you can run all the tests during the package build, since the necessary test files are not part of the package), instead I build it as it is. I have created a new conda environment for testing the package, and then I install it with:

conda install -c conda-forge --use-local libneuroml

Because of the --use-local, it will use the package that I've previously built. I then run the tests in neuroml/test directory of the source repository (with nosetests, but pytest it should work as well). As you said, you cannot install from pip in meta.yaml -- I simply removed the conda package for jsonpickle in my test environment and installed the older version with pip install jsonpickle==0.7.2.

pgleeson · 2018-06-12T13:17:02Z

@kapilkd13 I agree that the case fraction_along != 0 or 1 could be ignored, as it would complicate the conversion of the model to Brian & not produce correct electrical connectivity, but the case fraction_along = 0 should be fine: it explicitly states that the new segment is connected to the proximal point of the parent, and that should be fine to represent in Brian format.

mstimberg · 2018-06-12T13:24:04Z

@pgleeson I agree that Brian does support it in principle. The reason why I told @kapilkd13 to ignore it for now is that the current approach for converting the NeuroML morphology to Brian is to first convert it into a point-based format like the one used by SWC, so that we can then use Brian's existing import facility for this representation. With that SWC-like format, we only have a simple "parent" link, so we cannot express the difference between a proximal and distal connection.

kapilkd13 · 2018-06-13T17:00:50Z

Hi @mstimberg I tried few things to resolve jsonpickle issue but wasn't very successful. libneuroml is using it for a very trivial task of loading json and it shouldn't throw this error. I do found an active issue on jsonpickle repo jsonpickle/jsonpickle#192 related to this. I guess its some issue with jsonpickle itself..

mstimberg · 2018-06-14T14:05:31Z

Hi @kapilkd13 , I looked into this in quite a bit of detail and added my comments to the github issue you linked. The specific issue seems to be a problem in jsonpickle itself, but there's also an option that we can enable in newer jsonpickle versions that does the trick: NeuralEnsemble/libNeuroML/pull/72

Note that during testing, I had to clear the database, otherwise it would not overwrite the incorrect JSON with the fixed version:

>>> import pymongo
>>> client = pymongo.MongoClient('localhost', 27017)
>>> client.drop_database('mike_test_db3')
>>> client.drop_database('test_db_4')

kapilkd13 · 2018-06-14T14:25:35Z

@mstimberg I saw you comment on that thread, you dived in the code base and got to the issue. That was awesome :) I will try your suggestion. I also want to ask if I should push segment group related commits on this branch only or should we get this merge and start our work on new branch.

mstimberg · 2018-06-14T14:31:08Z

I also want to ask if I should push segment group related commits on this branch only or should we get this merge and start our work on new branch.

I'd say: until we have a libneuroml package on conda-forge (and therefore can run the automatic test suite in its current form) you can add your changes here. As soon as this is the case, we can finish this branch and merge it, and then start to work on new features in separate pull requests.

mstimberg · 2018-06-14T16:34:52Z

Can you have another go at the conda-forge recipe with the updated version of libneuroml that Padraig just uploaded to pypi (https://pypi.org/project/libNeuroML/)? It should now hopefully work with a recent version of jsonpickle. If it does, I think it is ready to for a pull request in conda-forge (you could maybe squash all commits into one before proposing it, though).

kapilkd13 · 2018-06-15T10:56:29Z

@mstimberg I tried with new libneuroml version but I am still getting the error. I modified meta.yaml file, updated libneuroml repo for any new changes, dropped mongo table. Then I created a new conda env for testing but I am still getting these errors. Can you test it on your system and verify.

kapilkd13 · 2018-06-15T11:41:07Z

@mstimberg I have created a new branch for conda recipe. Here is a link to final recipe https://github.com/kapilkd13/staged-recipes/blob/add_libneuroml/recipes/libneuroml/meta.yaml
Everything is working fine now. Actually I forgot to switch to development branch after pulling changes.

mstimberg · 2018-06-15T11:51:13Z

Great, I can confirm that it works fine on my machine as well. Please go ahead and open the pull request for inclusion in conda-forge.

kapilkd13 · 2018-06-16T18:53:39Z

Hi @mstimberg I was working on morphology function. I have a doubt, while reading a nml file how should we determine if the given set of segments belong to a section. What I am thinking is to create a tree of morphology object and perform DFS and consider link of segments that have only one children (ex. A--B--C--D) as one section. Should we do this for intermediate nodes also. I mean
A-----B-----C---D---E----G
         |              |
        C             F
In this tree with soma at A, we can consider E---G as one section, but should we also consider C--D as one section as C has only one child, giving us
A-----B-------sec1---sec2
         |              |
        C             F
Also should A---B be considered as one section

mstimberg · 2018-06-17T12:26:03Z

Yes, I'd consider the second to be the right choice, with A and B separate if A is named soma and B something else. When we import from SWC, our policy is: merge every segment that only has one child with its child segment if both segments have the same type. In NeuroML we don't have a direct equivalent to the type information, but we can use a heuristic based on the name of the segment. I'd propose two options (e.g. with a name_heuristic option that defaults to True):

name_heuristic == False: Ignore the name of the segments, merge all segments if they only have one child. The only exception: if a segment is called soma, don't merge it with anything.
name_heuristic == True: Merge segments if they only have one child and their name matches, except for a numerical suffix. E.g., dend0 and dend1 can be merged but dend0 and apic0 cannot.

Hope that makes sense?

kapilkd13 · 2018-06-19T16:17:00Z

Hi @mstimberg I have pushed current development, moving towards class implementation and changing morphology generation method. Its pretty rough right now but it will be the basic structure for us. please have a look.
Command like this should work

from brian2tools.nmlimport.nml import *
s=MorphologyTree('nml-file')
plot_morphology(s.morphology_obj)

kapilkd13 · 2018-07-05T02:57:22Z

ah, I forgot to rebase my branch against master. Docs are building now, Thanks :) Please go ahead and review this PR. In the meantime I will look at the biophysical properties extracted from the file.

mstimberg

All looks very good in principle, I left quite a number of comments but these are mostly style issues that should be easy to fix.

mstimberg · 2018-07-06T11:50:00Z

brian2tools/nmlimport/helper.py

+from collections import defaultdict
+
+# Return segment type depending on proximal and distal diameter
+def get_segment_type(segment):


I think this function is not used anymore.

mstimberg · 2018-07-06T11:50:29Z

brian2tools/nmlimport/helper.py

+
+
+# Checks if distal of first segment connects with proximal of second segment
+def are_segments_joined(segment1, segment2):


As discussed, I don't think we need this check anymore ("unconnected" segments are fine).

mstimberg · 2018-07-06T11:50:47Z

brian2tools/nmlimport/helper.py

+
+
+# Shift coordinates for further processing
+def shift_coords(x):


Another function that is not used.

mstimberg · 2018-07-06T11:52:20Z

brian2tools/nmlimport/nml.py

@@ -0,0 +1,349 @@
+import neuroml.loaders as loaders


Purely cosmetic, but I prefer imports to be ordered/grouped (first standard library imports, then 3-rd party, then brian2/brian2tools), see http://brian2.readthedocs.io/en/stable/developer/guidelines/style.html

mstimberg · 2018-07-06T11:53:03Z

brian2tools/nmlimport/nml.py

+
+                seg_parent = get_parent_segment(segment, segments)
+
+                if not are_segments_joined(segment, seg_parent):


As mentioned in the helper file, it is now fine if segments are not joined.

mstimberg · 2018-07-06T12:28:59Z

docs_sphinx/user/nmlimport.rst

+    morphology=nml_object.morphology_obj
+|
+
+- To obtain topology information.


Maybe this section could be shortened a bit. How about saying: "With this Morphology object, you can use all of Brian's functions to get information about the cell:" and then having a long code block (with these interactive >>> prompts). I don't think you need to say that morphology.area gives you the area, etc.

mstimberg · 2018-07-06T12:30:58Z

docs_sphinx/user/nmlimport.rst

+   Pyramidal cell's Morphology plot.
+
+
+- To get the resolved group ids of an nml `SegmentGroup`. This returns a


As discussed, I think it would be good to first show here a shortened version of the NeuroML file. I'd also add a little explanation of what a SegmentGroup is. BTW: You should use two backticks, i.e. ``SegmentGroup`` because SegmentGroup is not an object defined anywhere in the documentation.

mstimberg · 2018-07-06T12:31:25Z

docs_sphinx/user/nmlimport.rst

+    [8, 6, 7], 'basal_gaba_input': [6], 'background_input': [7]}
+|
+
+- To get the information of all the child segments of a parent segment. This


Not sure we need this, you can also use the children information stored in Brian's morphology

mstimberg · 2018-07-06T12:32:19Z

docs_sphinx/user/nmlimport.rst

+        </morphology>
+    </cell>
+
+Handling FractionAlong value


I would rather explain this in the section title (e.g. "Handling sections not connected at the distal end")

mstimberg · 2018-07-06T12:32:38Z

docs_sphinx/user/nmlimport.rst

+    </cell>
+
+Handling FractionAlong value
+--------------------------------


Underline needs to have the same length as the title 😄

kapilkd13 · 2018-07-10T21:36:41Z

Hi @mstimberg , I have made the suggested changes. But due to some reason travis is failing for python2. It looks like there is some issue with conda build in python2. Please have a look at https://travis-ci.org/brian-team/brian2tools/jobs/402402431

mstimberg · 2018-07-11T05:03:55Z

But due to some reason travis is failing for python2. It looks like there is some issue with conda build in python2.

There was a problem in the conda package itself, but it got updated a few hours ago. I restarted the tests and they now pass.

mstimberg

Everything is looking very good, there are just a few more typos and missing NmlMorphology->NMLMorphology renames to correct (see my comments).

mstimberg · 2018-07-11T11:52:01Z

brian2tools/nmlimport/nml.py

+        return doc
+
+
+    def _is_heurestically_sep(self, section, seg_id):


"heurestically" --> "heuristically"

mstimberg · 2018-07-11T11:52:38Z

brian2tools/nmlimport/nml.py

+        Returns
+        -------
+        bool
+            Returns true if the given segment is not heurestically connected


mstimberg · 2018-07-11T11:54:23Z

brian2tools/nmlimport/nml.py

+
+    def __init__(self, file_obj, name_heuristic=True):
+        """
+        Initializes NmlMorphology Class


Here and in a few other places: "NmlMorphology" --> "NMLMorphology"

mstimberg · 2018-07-11T11:55:05Z

brian2tools/tests/testScript.py

@@ -0,0 +1,62 @@
+import argparse
+import os, sys
+from brian2tools.nmlimport.nml import NmlMorphology


This is now broken I guess (NmlMorphology --> NMLMorphology)

mstimberg · 2018-07-11T11:55:48Z

docs_sphinx/user/nmlimport.rst

+
+.. code:: python
+
+    from brian2tools.nmlimport.nml import NmlMorphology


NmlMorphology --> NMLMorphology (here and below in a few places)

kapilkd13 · 2018-07-12T15:41:36Z

Hi @mstimberg I was developing function to convert str values like cond_density to quantity object and then I found https://github.com/brian-team/brian2tools/blob/master/brian2tools/nmlexport/supporting.py#L12 this method 😃 . This converts our string value to quantity object. I tested with few values, result looks good. We can make a copy of that function and extend it for our usecase. What do you think?
Something like this does the work:

from brian2tools.nmlexport.supporting import from_string
m=NMLMorphology('.nml file')
q=from_string(m.doc.cells[0].biophysical_properties.membrane_properties.channel_densities[0].cond_density)

mstimberg · 2018-07-13T13:23:31Z

Hi @mstimberg I was developing function to convert str values like cond_density to quantity object and then I found https://github.com/brian-team/brian2tools/blob/master/brian2tools/nmlexport/supporting.py#L12 this method smiley . This converts our string value to quantity object. I tested with few values, result looks good. We can make a copy of that function and extend it for our usecase. What do you think?

Ha, I completely forgot about this function, I thought we only had the conversion in the other direction! We need to test this function more thoroughly, but in principle it should do what we want. So indeed no need to write this function again. Maybe the best approach would be to have a new package in parallel to nmlexport and nmlimport that includes functions that are useful for both, something like nmlutils.

mstimberg · 2018-07-13T13:25:47Z

...and merged it is! This is already very useful as it is, looking forward to seeing even more features implemented 👍

kapilkd13 mentioned this pull request May 24, 2018

Importing morphologies into Brian from a NeuroML file #15

Open

8 tasks

mstimberg reviewed Jun 11, 2018

View reviewed changes

kapilkd13 added 13 commits July 5, 2018 08:12

adding helper functions

8a71f46

modifying section name generation according to requirements

21bb391

generating resoled ids inside morphology object

a4229b2

moving functions to morphology class

cfbd559

alloing fraction along value of 0

6a55194

adding docstrings

4f411eb

modifying tests as per new model

01232cd

modifyying testscript

f5ec3c0

adding conda-forge as channel

3c77f56

adding libneuroml to run time requirements

95bbf6b

adding sample .nml files to package data

6e4322d

Adding user documentation

6a6ac54

adding pyramidal morphology plot

6116541

mstimberg requested changes Jul 6, 2018

View reviewed changes

kapilkd13 added 4 commits July 9, 2018 22:38

style changes and removing unused code

f540809

renaming class and modifying test file

c8e342b

modifying docstrings

74cac93

improving user documentation

8fad34a

kapilkd13 force-pushed the nmlimport branch from 9e2f7b8 to 8fad34a Compare July 10, 2018 20:55

mstimberg requested changes Jul 11, 2018

View reviewed changes

kapilkd13 added 2 commits July 11, 2018 21:53

correcting typos and changing NmlMorphology to NMLMorphology

ad3b648

minor fix

5fa0eb2

mstimberg approved these changes Jul 13, 2018

View reviewed changes

mstimberg merged commit 339376e into brian-team:master Jul 13, 2018



		# Checks if distal of first segment connects with proximal of second segment
		def are_segments_joined(segment1, segment2):



		# Shift coordinates for further processing
		def shift_coords(x):


		seg_parent = get_parent_segment(segment, segments)

		if not are_segments_joined(segment, seg_parent):

		Pyramidal cell's Morphology plot.


		- To get the resolved group ids of an nml `SegmentGroup`. This returns a


		.. code:: python

		from brian2tools.nmlimport.nml import NmlMorphology

[WIP] nmlimport package: importing morphologies from nml file #17

[WIP] nmlimport package: importing morphologies from nml file #17

Conversation

kapilkd13 commented May 23, 2018

kapilkd13 commented May 28, 2018

mstimberg commented May 28, 2018 • edited Loading

kapilkd13 commented May 29, 2018

mstimberg commented May 31, 2018

kapilkd13 commented Jun 1, 2018 • edited Loading

kapilkd13 commented Jun 7, 2018

mstimberg commented Jun 7, 2018

kapilkd13 commented Jun 8, 2018

mstimberg commented Jun 8, 2018

kapilkd13 commented Jun 10, 2018

mstimberg commented Jun 10, 2018

kapilkd13 commented Jun 11, 2018

mstimberg commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kapilkd13 commented Jun 11, 2018

mstimberg commented Jun 11, 2018

pgleeson commented Jun 12, 2018

mstimberg commented Jun 12, 2018

kapilkd13 commented Jun 13, 2018

mstimberg commented Jun 14, 2018

kapilkd13 commented Jun 14, 2018

mstimberg commented Jun 14, 2018

mstimberg commented Jun 14, 2018

kapilkd13 commented Jun 15, 2018

kapilkd13 commented Jun 15, 2018

mstimberg commented Jun 15, 2018

kapilkd13 commented Jun 16, 2018 • edited Loading

mstimberg commented Jun 17, 2018

kapilkd13 commented Jun 19, 2018

kapilkd13 commented Jul 5, 2018

mstimberg 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

kapilkd13 commented Jul 10, 2018

mstimberg commented Jul 11, 2018

mstimberg 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

kapilkd13 commented Jul 12, 2018

mstimberg commented Jul 13, 2018

mstimberg commented Jul 13, 2018

mstimberg commented May 28, 2018 •

edited

Loading

kapilkd13 commented Jun 1, 2018 •

edited

Loading

kapilkd13 commented Jun 16, 2018 •

edited

Loading