[MRG] Find end pts of section and a numpy version of h.distance() #661

raj1701 · 2023-07-03T01:40:35Z

No description provided.

raj1701 · 2023-07-03T01:45:25Z

hnn_core/find_coords.py

+    for section_name, section in cell.sections.items():
+        print(section_name)
+        print(section.end_pts)


The end_pts printed here are different from those defined in cells_default for cell L2Pyr.

Do I simply need to replace these end_pts in cells_default or should I build the cell and find them using cell._nrn_sections

You need to build the cell and get the coords from that

When any new cell is created using Cell class, a private function __update_end_pts() is called at the end of __init__. This function creates neuron sections by calling _create_sections() then updates the end_pts of the section. The neuron objects are then deleted using del self._nrn_sections[name].

I have a few questions regarding this flow -

Neuron objects are created and destroyed in this flow which according to the discussion in last meeting should be avoided outside of simulation.

If this functionality is already there and the end_pts are automatically updated, is manually updating the end_pts required ?

Should this function _update_end_pts() be removed?

Also if neuron objects can be cleared using del, can't we follow a similar scheme while saving the network? After calling build on the copy of a cell, can we just iteratively delete the _nrn_sections and _nrn_synapses of the built cell??

The problem is that Neuron objects are wrappers around C objects ... when del is called, it makes a request to the garbage collector to clear the object from memory. It's a request, not an order. If a pointer is not cleared from memory because of how it is coded in C, it will lead to a segmentation fault and/or slow down the following simulation. The latter will be very hard to debug ... we do have a scheme to clear neuron objects in NetworkBuilder but it was contributed by someone really in the weeds who understands Neuron.

But you have a good point about _update_end_pts ... I think @ntolley implemented that. Not sure how well it's been tested if the del in that function does not lead to a performance regression. Any thoughts Nick?

codecov-commenter · 2023-07-03T01:48:45Z

Codecov Report

Merging #661 (dffc2ea) into master (c2ad435) will increase coverage by 0.06%.
The diff coverage is 97.56%.

❗ Current head dffc2ea differs from pull request most recent head 6c55cb7. Consider uploading reports for the commit 6c55cb7 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #661      +/-   ##
==========================================
+ Coverage   91.55%   91.62%   +0.06%     
==========================================
  Files          22       22              
  Lines        4405     4488      +83     
==========================================
+ Hits         4033     4112      +79     
- Misses        372      376       +4

Files Changed	Coverage Δ
hnn_core/cell.py	`96.53% <97.47%> (-0.43%)`	⬇️
hnn_core/cells_default.py	`98.33% <100.00%> (ø)`
hnn_core/optimization.py	`92.99% <100.00%> (ø)`

raj1701 · 2023-07-08T05:07:27Z

As discussed in the meeting, I stored a base network and dipole and checked equality of them after removing _update_end_pts() and h.define_shape() . Here are some observations -

It seems that both _update_end_pts() and h.define_shape() are required in order to obtain the final end_pts. Removal of either one of them lead to a mismatch
The reason is _update_end_pts() is dependent on h.define_shape(). Also the end_pts are only updated once when the cell is initialised. This function is only used by modify_section() which itself is not used anywhere.

raj1701 · 2023-07-08T05:11:31Z

I think if all end_pts are updated in cells_default, building any neuron objects before simulation may be avoided. I will try this now.

hnn_core/cell.py

jasmainak · 2023-08-07T18:02:55Z

@raj1701 let me know when I should look !

hnn_core/cell.py

jasmainak

I spent some time reading the code ... everything isn't fully clear to me yet but it's getting there! Let's iterate a bit more so we can clean it up nicely. I'm passing the ball back to you @raj1701 . Let me know as soon as you do your pass (in a day or two?) and I'll read it one more time

hnn_core/cell.py

jasmainak · 2023-08-09T20:43:32Z

hnn_core/cell.py

+                dist_1 = self.distance_section(target_sec_name,
+                                               sec_name, sec_end_pt)


Suggested change

dist_1 = self.distance_section(target_sec_name,

sec_name, sec_end_pt)

dist_1 = self.distance_section(target_sec_name, node)

I don't fully follow the logic of these if/else statements

The algorithm searches for the destination nodes (end 0 or 1 of the target section) in all available paths. If the target section is in the subtree of the current node dist_1 (now named it dist_temp) will contain distance between the current node to the centre of the target section

hnn_core/cell.py

rythorpe · 2023-08-09T20:50:43Z

This looks like a recreation of the NEURON functionality, where the cell sections are first defined with endpoints, attached to each other into a "tree", and then morphed according a specified length parameter for each section. This is exactly what we're trying to avoid, which can be accomplished by specifying the final endpoints to begin with.

jasmainak · 2023-08-09T20:57:23Z

@rythorpe this was done so as to avoid calling Neuron functions when saving the network ... right now, it's all implemented in pure python, so the ionic concentrations are known without using Neuron. I remember you were concerned about dangling neuron objects. This takes care of that issue. Furthermore, we better understand how the neuron distance function works :)

jasmainak · 2023-08-09T20:59:20Z

Perhaps you imagined hardcoding the lengths/coordinates ... but that's not possible because we already had functionality to change the lengths (implemented by @ntolley ). The only way to match everything up was to reimplement it in Python

rythorpe · 2023-08-09T21:01:11Z

Yes, I thought the whole goal was just to update the hardcoded endpoints because it'd be the most lightweight solution. I don't understand why we needed to re-implement everything in Python.

jasmainak · 2023-08-09T21:03:43Z

hardcoding wouldn't have achieved what you wanted because of:

hnn-core/hnn_core/cell.py

Lines 682 to 697 in c81e5b8

    
           def _update_end_pts(self): 
        
               """"Create cell and copy coordinates to Section.end_pts""" 
        
               self._create_sections(self.sections, self.topology) 
        
               section_names = list(self.sections.keys()) 
        
               for name in section_names: 
        
                   nrn_pts = self._nrn_sections[name].psection()['morphology'][ 
        
                       'pts3d'] 
        
                   del self._nrn_sections[name] 
        
                   x0, y0, z0 = nrn_pts[0][0], nrn_pts[0][1], nrn_pts[0][2] 
        
                   x1, y1, z1 = nrn_pts[1][0], nrn_pts[1][1], nrn_pts[1][2] 
        
                   self.sections[name]._end_pts = [[x0, y0, z0], [x1, y1, z1]] 
        
               self._nrn_sections = dict()

see it already calls neuron functions. You'd have deprecate this ...

ntolley · 2023-08-09T23:06:07Z

hnn_core/tests/test_cell.py

+    cell1.update_end_pts()
+    for sec_name in cell1.sections.keys():
+        section = cell1.sections[sec_name]
+        section._L = section._L / 2


Why are we dividing by two after calling update endpoints? wouldn't this lead to a mismatch?

Definitely think this is the right format for the test, and the explicit checks below are good too, but not sure I totally follow all the logic

We divide all section lengths by 2 to get back the original end points because earlier we had doubled them and updated end_pts in line 117.

hnn_core/cell.py

hnn_core/cells_default.py

jasmainak · 2023-08-21T19:40:31Z

hnn_core/cell.py

+                    if hasattr(val, '__call__'):
+                        seg_xs, seg_vals = list(), list()
+                        section_distance = self.distance_section(sec_name,
+                                                                 ('soma', 0))
+                        adjacent_seg_dist = 1 / section.nseg
+                        first_seg_centre = (0.5 - (((section.nseg - 1) / 2) *
+                                            adjacent_seg_dist))
+                        seg_centres = list()
+
+                        # Finding centres of all segments in the section
+                        # If number of segments is 5 then seg_centres will
+                        # be 0.1, 0.3, 0.5, 0.7 and 0.9.
+                        for i in range(0, section.nseg):
+                            seg_centres.append(first_seg_centre +
+                                               (i * adjacent_seg_dist))
+
+                        for seg_x in seg_centres:
+                            # sec_end_dist is distance between 0 end of soma to
+                            # the 0 or 1 end of section (whichever is closer)
+                            sec_end_dist = section_distance - (section.L / 2)
+                            seg_xs.append(seg_x)
+                            seg_vals.append(val(sec_end_dist +
+                                                (seg_x * section.L)))
+                        p_mech[attr] = [seg_xs, seg_vals]


can this logic be moved here: https://github.com/jonescompneurolab/hnn-core/blob/master/hnn_core/cells_default.py ?

So we don't use the partial in the first place and directly store the values when creating the cell.

jasmainak · 2023-08-21T19:42:27Z

hnn_core/cell.py

+                        adjacent_seg_dist = 1 / section.nseg
+                        first_seg_centre = (0.5 - (((section.nseg - 1) / 2) *
+                                            adjacent_seg_dist))
+                        seg_centres = list()
+
+                        # Finding centres of all segments in the section
+                        # If number of segments is 5 then seg_centres will
+                        # be 0.1, 0.3, 0.5, 0.7 and 0.9.
+                        for i in range(0, section.nseg):
+                            seg_centres.append(first_seg_centre +
+                                               (i * adjacent_seg_dist))


any chance we can use np.linspace ? That seems exactly for this purpose

Very slight precision errors are there. But it is passing the check in _set_biophysics()

hnn_core/cells_default.py

hnn_core/tests/test_cell.py

hnn_core/cell.py

jasmainak · 2023-08-21T19:53:15Z

hnn_core/cell.py

+                    if isinstance(val, list):
+                        seg_xs, seg_vals = val[0], val[1]
+                        for seg, seg_x, seg_val in zip(sec, seg_xs, seg_vals):
+                            # Checking equaliy till 5 decimal places
+                            np.testing.assert_almost_equal(seg.x, seg_x, 5)
+                            setattr(seg, attr, seg_val)


let's be brave and delete _set_biophysics

Don't we still need this function for inserting nrn mechanisms and such?

I think _set_section_mechs is doing that now. This method only exists for checking equality with the old stuff

I think it is still required as it is inserting mechanisms in the NEURON section objects.
_set_section_mechs() is just expanding the partial functions for now. If we include the _set_biophysics() logic here then NEURON objects might still be created before simulating the network (in case of network write)

ntolley · 2023-08-23T16:38:24Z

I'm a bit stumped as to why this is only failing on windows...

hnn_core/cell.py

ntolley · 2023-08-24T13:38:05Z

hnn_core/cell.py

+                             self.sections[node[0]].L)
+            else:
+                dist_temp = self.distance_section(target_sec_name, node)
+            dist = min(dist, dist_temp)


Suggested change

dist = min(dist, dist_temp)

dist = np.nanmin(dist, dist_temp)

It throws a warning when all nan values are passed to the function. The results are correct. This warning is fine right?

For now I have added a check to resolve this issue.

ntolley · 2023-08-24T13:38:21Z

hnn_core/cell.py

+            raise TypeError("distance_section() "
+                            "cannot work with cell_tree as None.")
+        if curr_node not in self.cell_tree:
+            return 1000000


Suggested change

return 1000000

return np.nan

jasmainak · 2023-08-24T19:00:33Z

@raj1701 let us know when we can look again!

raj1701 · 2023-08-24T20:33:13Z

Hey @jasmainak @ntolley @rythorpe , I have rebased and pushed the final code. Please take a look

jasmainak

Looks good to me! @rythorpe or @ntolley do you want to merge?

rythorpe · 2023-08-24T23:14:52Z

I won't be able to look this over until tomorrow afternoon. If we don't feel like waiting until then, feel free to merge without me @ntolley.

jasmainak · 2023-08-25T14:38:55Z

I went ahead and merged to keep the momentum! @raj1701 let's bring your other PR to merge state as soon as possible :)

raj1701 commented Jul 3, 2023

View reviewed changes

jasmainak reviewed Aug 3, 2023

View reviewed changes

hnn_core/cell.py Outdated Show resolved Hide resolved

jasmainak reviewed Aug 3, 2023

View reviewed changes

hnn_core/cell.py Outdated Show resolved Hide resolved

jasmainak reviewed Aug 3, 2023

View reviewed changes

hnn_core/cell.py Outdated Show resolved Hide resolved

ntolley reviewed Aug 9, 2023

View reviewed changes

hnn_core/cell.py Show resolved Hide resolved

jasmainak requested changes Aug 9, 2023

View reviewed changes

ntolley reviewed Aug 9, 2023

View reviewed changes

ntolley reviewed Aug 10, 2023

View reviewed changes

hnn_core/cell.py Outdated Show resolved Hide resolved

raj1701 requested a review from jasmainak August 12, 2023 02:33

ntolley reviewed Aug 14, 2023

View reviewed changes

hnn_core/cell.py Show resolved Hide resolved

ntolley mentioned this pull request Aug 17, 2023

generalized_opt_routine #652

Merged

jasmainak reviewed Aug 21, 2023

View reviewed changes

hnn_core/cells_default.py Show resolved Hide resolved

jasmainak reviewed Aug 21, 2023

View reviewed changes

hnn_core/cells_default.py Show resolved Hide resolved

jasmainak reviewed Aug 21, 2023

View reviewed changes

hnn_core/tests/test_cell.py Outdated Show resolved Hide resolved

jasmainak reviewed Aug 21, 2023

View reviewed changes

hnn_core/tests/test_cell.py Outdated Show resolved Hide resolved

jasmainak reviewed Aug 21, 2023

View reviewed changes

hnn_core/cell.py Outdated Show resolved Hide resolved

jasmainak reviewed Aug 21, 2023

View reviewed changes

hnn_core/cell.py Outdated Show resolved Hide resolved

jasmainak reviewed Aug 21, 2023

View reviewed changes

jasmainak reviewed Aug 24, 2023

View reviewed changes

hnn_core/cell.py Outdated Show resolved Hide resolved

jasmainak requested review from rythorpe and ntolley August 24, 2023 01:09

ntolley reviewed Aug 24, 2023

View reviewed changes

hnn_core/cell.py Outdated Show resolved Hide resolved

ntolley reviewed Aug 24, 2023

View reviewed changes

hnn_core/cell.py Outdated Show resolved Hide resolved

ntolley reviewed Aug 24, 2023

View reviewed changes

hnn_core/cell.py Outdated Show resolved Hide resolved

ntolley reviewed Aug 24, 2023

View reviewed changes

raj1701 added 12 commits August 24, 2023 20:27

Printing end_pts

ed1eaff

Updated neuron end_pts

31677a5

Added python version of h.define_shape()

a9ad309

Cleaned code

d5ef00e

Added tests and made distance function more readable

3123509

Separated the functionality of set_biophysics and set_section_mech

ac2c0ab

Made cell tree code more readable

fb11660

Refactored code

72396da

Removed topology

49eac18

Attempt at removing testing errors

3c92fcf

Addressed comments

8952d98

Refactored code

6c55cb7

raj1701 force-pushed the neuron_coordinates branch from 492b6dc to 6c55cb7 Compare August 24, 2023 20:31

jasmainak approved these changes Aug 24, 2023

View reviewed changes

jasmainak merged commit 2f5b15b into jonescompneurolab:master Aug 25, 2023
7 checks passed

raj1701 mentioned this pull request Aug 26, 2023

[ENH] Added read/write method for network object #651

Merged

This was referenced Jul 10, 2024

simplify cell morphology with updated segment coordinates #659

Closed

separating the geometry of cells/networks from the Neuron code #165

Closed

		dist_1 = self.distance_section(target_sec_name,
		sec_name, sec_end_pt)

	dist = min(dist, dist_temp)
	dist = np.nanmin(dist, dist_temp)

[MRG] Find end pts of section and a numpy version of h.distance() #661

[MRG] Find end pts of section and a numpy version of h.distance() #661

Conversation

raj1701 commented Jul 3, 2023

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-commenter commented Jul 3, 2023 • edited Loading

Codecov Report

raj1701 commented Jul 8, 2023

raj1701 commented Jul 8, 2023

jasmainak commented Aug 7, 2023

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

rythorpe commented Aug 9, 2023

jasmainak commented Aug 9, 2023

jasmainak commented Aug 9, 2023

rythorpe commented Aug 9, 2023 • edited Loading

jasmainak commented Aug 9, 2023

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

ntolley commented Aug 23, 2023

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 commented Aug 24, 2023

raj1701 commented Aug 24, 2023 • edited Loading

jasmainak left a comment

Choose a reason for hiding this comment

rythorpe commented Aug 24, 2023

jasmainak commented Aug 25, 2023

codecov-commenter commented Jul 3, 2023 •

edited

Loading

rythorpe commented Aug 9, 2023 •

edited

Loading

raj1701 commented Aug 24, 2023 •

edited

Loading