MRG: Improve cells #200

jasmainak · 2020-10-29T18:47:58Z

related to #174 and #165

jasmainak · 2020-10-29T18:51:29Z

hnn_core/pyramidal.py

+    geometry : dict | str | None
+        The parameters dictionary containing geometry.
+        If it's a string, then load the geometry from
+        a .geo file. If None, load the default geometry


I was mulling over this. @rythorpe let me know your thoughts before I go too deep trying to implement it. Does this kind of API make sense? Or should it be rather a parameter of set_geometry ? Looking at these .geo files (e.g., here) it seems they only define the dendrite locations and the connections. The synapses would still have to be manually defined by the user. Note I am trying to go for something that is relatively easy to implement but also serves a practical purpose ...

This looks really good actually because of the modular flexibility it will lend to pyramidal cells. I hadn't heard of .geo files before this but I'm assuming they are the standard file type for storing dendritic geometry in nrn hoc?

I'm assuming the geometry argument will be passed down from higher-level geometry arguments of the Network and NetworkBuilder classes in order to make it accessible to the user?

I hadn't heard of .geo files before this but I'm assuming they are the standard file type for storing dendritic geometry in nrn hoc?

I think so, but even if they are not ... I think the modularization would pave the way for other loaders and should be rather straigthforward. A quick google search also revealed swc files. It should also be straightforward to implement once we have this in place.

I'm assuming the geometry argument will be passed down from higher-level geometry arguments of the Network and NetworkBuilder classes in order to make it accessible to the user?

You asked the tough question :) I don't think it would be enough to pass down a geometry argument from Network or NetworkBuilder because a user would still need to define mechanisms, synapses, and how other cells connect to this cell. What might work is allowing the user to define the Pyramidal cell class/objects, passing them to Network/NetworkBuilder and cloning them to create a network. Wdyt?

What might work is allowing the user to define the Pyramidal cell class/objects, passing them to Network/NetworkBuilder and cloning them to create a network. Wdyt?

That's an interesting idea, making the child cell classes (i.e., L2Pyr, L5Pyr, and BasketSingle) accessible to the user. If we do that we'll need to refactor the cell object constructor and other methods so that nrn objects related to the network are only called after the user passes them into Network/NetworkBuilder.

hnn_core/pyramidal.py

jasmainak · 2020-10-30T02:37:19Z

hnn_core/pyramidal.py

@@ -372,10 +402,8 @@ def topol(self):
        self.dends['basal_2'].connect(self.dends['basal_1'], 1, 0)
        self.dends['basal_3'].connect(self.dends['basal_1'], 1, 0)

-    def _biophys_soma(self):
+    def _biophysics(self):


this makes it consistent with basket cells. Also I think it's just nicer to have all the biophysics defined in one place.

jasmainak · 2020-10-30T02:38:25Z

hnn_core/pyramidal.py

@@ -430,11 +465,11 @@ class L5Pyr(Pyr):
        List of dendrites.
    """

-    def __init__(self, gid=-1, pos=-1, p={}):


defining p={} is an anti-pattern. You can get surprising results. See here

ntolley · 2020-10-30T22:37:08Z

hnn_core/cell.py

-        secloc : float (0. to 1.0)
-            The section location
+        secloc : instance of nrn.Segment
+            The section location. E.g., soma(0.5).


Describing nrn objects with floats and ints shows up in several docstrings. Is this just referencing the numerical argument that instantiates the object?

it might be documented wrongly :-) fix is always welcome!

btw, soma(0.5) just means the middle of the soma ... I guess you know that already but putting it out there in case that was not clear.

codecov-io · 2020-11-03T03:32:24Z

Codecov Report

Merging #200 (93b57b3) into master (bb559b7) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
- Coverage   66.89%   66.83%   -0.07%     
==========================================
  Files          20       20              
  Lines        2039     1975      -64     
==========================================
- Hits         1364     1320      -44     
+ Misses        675      655      -20

Impacted Files	Coverage Δ
hnn_core/basket.py	`100.00% <100.00%> (ø)`
hnn_core/cell.py	`77.08% <100.00%> (+8.25%)`	⬆️
hnn_core/pyramidal.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb559b7...93b57b3. Read the comment docs.

jasmainak

@rythorpe @ntolley @blakecaldwell this is good to go for me. I backtracked a bit on loading arbitrary cells. It seems we would benefit more from clarifying and strengthening the object structure. Here is a summary of the changes:

consolidate the init of L2Pyr and L5Pyr in the parent class
remove get_sectnames method and replace with hard-coded dictionary in Cell.secs(). This has the advantage of not depending on Neuron.
Remove list_all as an attribute of cell class. We should not introduce arbitrary attributes. They should only be introduced in the init
Remove p_all as an attribute to avoid dependencies in the code on default shape
Move hard coded topology into a nested list structure. All the geometry related information is going into Cell.secs() method. The data structures are not final but moving them to one place helps separate hard-coded values.
Finally, there are a number of related yet confusingly similar attributes ... some give you dendrite names, some section names, some Neuron sections etc. We might want to carefully think and consolidate them only into ones that we need. But I leave that for another PR.

rythorpe · 2020-11-05T23:06:42Z

hnn_core/pyramidal.py

@@ -49,30 +73,36 @@ def __init__(self, gid, soma_props):
        self.sect_loc = dict()
        # for legacy use with L5Pyr
        self.list_dend = []


In the spirit of cleaning up cell class attributes, is Pyr.list_dend necessary given that we have the keys in Pyr.dends?

I guess we can save this for another PR as you mentioned above....

rythorpe · 2020-11-05T23:13:23Z

hnn_core/pyramidal.py

+            # XXX: risky to use self.soma as default. Unfortunately there isn't
+            # a dictionary with all the sections (including soma)


Given that self.create_soma() is called prior, why is this risky?

maybe not risky right now ... but if we loaded a geo or swc cell from the internet and it somehow was missing a section named soma, it could lead to a crash and a cryptic error. Unlikely but probable? I can remove the comment if you think it's more confusing than helpful

Good point. Let's keep the comment for now until we can implement input validation for .geo or .swc cell files.

rythorpe · 2020-11-05T23:25:50Z

hnn_core/pyramidal.py

-    def __biophys_soma(self):
-        # set soma biophysics specified in Pyr
-        # self.pyr_biophys_soma()
+    def _biophysics(self, p_all):


This is nitpicky, but could we rename this to _set_biophysics() or something along that line just to clarify that this modifies object attributes? Also, should we consider some sort of explicit or implicit method-naming convention that indicates that calls to neuron.h are occurring within (e.g., I think we created a similar convention for NetworkBuilder.build())?

totally, I made it set_biophysics so it mirrored set_geometry. I am imagining that after clean up, we will be left with one or two methods which make Neuron calls. Next, we'll need to make sure that no Neuron related attributes are instantiated when the class is instantiated. Getting closer to picklable cell class :)

rythorpe · 2020-11-05T23:32:50Z

Also, do we need the _Cell.move_to_pos() method? It seems unnecessary unless we want to completely hide the ability to translate cell position at the network level (in which case, maybe we can consolidate _Cell.move_to_pos() with _Cell.translate_to()).

jasmainak · 2020-11-06T05:12:15Z

Also, do we need the _Cell.move_to_pos() method? It seems unnecessary unless we want to completely hide the ability to translate cell position at the network level (in which case, maybe we can consolidate _Cell.move_to_pos() with _Cell.translate_to()).

actually it seems it has no impact on the results and it unnecessarily makes more Neuron calls. So I got rid of this. Okay for you?

jasmainak · 2020-11-06T05:15:29Z

@rythorpe comments addressed

rythorpe · 2020-11-06T18:42:04Z

actually it seems it has no impact on the results and it unnecessarily makes more Neuron calls. So I got rid of this. Okay for you?

Oh I see, so the only time cell positions are set are in the cell constructors during network instantiation in NetworkBuilder?

jasmainak · 2020-11-06T18:44:16Z

I take back my words ... the tests were failing, not sure why. Would need to dig in. But for now, I put it back with the consolidated method you suggested.

jasmainak · 2020-11-06T18:45:03Z

hnn_core/cell.py


-    def move_to_pos(self):
-        """Move cell to position."""
-        self.translate_to(self.pos[0] * 100, self.pos[2], self.pos[1] * 100)


I don't quite get this ... why pos[2] for y and why not pos[2] * 100

This is confusing. Why pos[2] instead of pos[1] for the y-coordinate?

Is the factor of 100 supposed to represent the actual distance between cells (i.e., scales the integer positions within the coordinate grid)?

I honestly don't know ... I raised an issue #202. Keeping it unchanged for now. Just reorganized the code.

rythorpe · 2020-11-06T22:08:55Z

Anyone else want to review or are we good to merge?

jasmainak · 2020-11-08T05:58:16Z

Feel free to merge if nobody pitches in a day or two :)

rythorpe · 2020-11-09T18:58:29Z

Feel free to merge if nobody pitches in a day or two :)

Merged! Thanks @jasmainak!

jasmainak added 4 commits October 29, 2020 13:33

ENH: remove unused methods

166c1ce

DOC: fix docstring

6650d81

MAINT: consolidate biophys for soma and dend into one method

5231256

WIP: try to load geometry

1d69221

jasmainak commented Oct 29, 2020

View reviewed changes

rythorpe reviewed Oct 30, 2020

View reviewed changes

hnn_core/pyramidal.py Outdated Show resolved Hide resolved

rythorpe reviewed Oct 30, 2020

View reviewed changes

hnn_core/pyramidal.py Outdated Show resolved Hide resolved

jasmainak commented Oct 30, 2020

View reviewed changes

ntolley reviewed Oct 30, 2020

View reviewed changes

jasmainak added 2 commits November 2, 2020 17:48

DOC more documentation and separating Neuron specific attributes

434a78e

Units

e3e6270

jasmainak added 5 commits November 2, 2020 23:06

Refactor topology

10c6d9c

Remove method for scaling

eeacc0c

Backtrack on reading geo files

b28a628

Remove function to get scales for real

ff7ba7e

init in parent class?

fdeb0e9

jasmainak changed the title ~~WIP: Improve cells~~ MRG: Improve cells Nov 3, 2020

jasmainak commented Nov 3, 2020

View reviewed changes

FIX scale

4495b26

rythorpe reviewed Nov 5, 2020

View reviewed changes

MAINT: _biophysics -> set_biophysics

8f96d1a

MAINT: consolidate move_to_pos

93b57b3

jasmainak force-pushed the improve_cells branch from e649944 to 93b57b3 Compare November 6, 2020 18:43

jasmainak commented Nov 6, 2020

View reviewed changes

jasmainak mentioned this pull request Nov 6, 2020

scaling of cell positions #202

Closed

jasmainak force-pushed the improve_cells branch from 65efea1 to 93b57b3 Compare November 6, 2020 22:00

rythorpe approved these changes Nov 6, 2020

View reviewed changes

rythorpe merged commit 83c5238 into jonescompneurolab:master Nov 9, 2020

jasmainak mentioned this pull request Apr 8, 2021

[MRG] MAINT: remove move_to_pos #314

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MRG: Improve cells #200

MRG: Improve cells #200

jasmainak commented Oct 29, 2020

jasmainak Oct 29, 2020 •

edited

Loading

rythorpe Oct 30, 2020

jasmainak Oct 30, 2020

rythorpe Oct 30, 2020

jasmainak Oct 30, 2020

jasmainak Oct 30, 2020

ntolley Oct 30, 2020

jasmainak Nov 1, 2020

codecov-io commented Nov 3, 2020 •

edited

Loading

jasmainak left a comment

rythorpe Nov 5, 2020

rythorpe Nov 5, 2020

rythorpe Nov 5, 2020

jasmainak Nov 6, 2020 •

edited

Loading

rythorpe Nov 6, 2020

rythorpe Nov 5, 2020

jasmainak Nov 6, 2020

rythorpe commented Nov 5, 2020

jasmainak commented Nov 6, 2020

jasmainak commented Nov 6, 2020

rythorpe commented Nov 6, 2020

jasmainak commented Nov 6, 2020

jasmainak Nov 6, 2020

rythorpe Nov 6, 2020

jasmainak Nov 6, 2020

rythorpe commented Nov 6, 2020

jasmainak commented Nov 8, 2020

rythorpe commented Nov 9, 2020

		# XXX: risky to use self.soma as default. Unfortunately there isn't
		# a dictionary with all the sections (including soma)

MRG: Improve cells #200

MRG: Improve cells #200

Conversation

jasmainak commented Oct 29, 2020

jasmainak Oct 29, 2020 • 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

Choose a reason for hiding this comment

codecov-io commented Nov 3, 2020 • edited Loading

Codecov Report

jasmainak 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

jasmainak Nov 6, 2020 • 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

rythorpe commented Nov 5, 2020

jasmainak commented Nov 6, 2020

jasmainak commented Nov 6, 2020

rythorpe commented Nov 6, 2020

jasmainak commented Nov 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rythorpe commented Nov 6, 2020

jasmainak commented Nov 8, 2020

rythorpe commented Nov 9, 2020

jasmainak Oct 29, 2020 •

edited

Loading

codecov-io commented Nov 3, 2020 •

edited

Loading

jasmainak Nov 6, 2020 •

edited

Loading