Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] Make cell morphology plots reflect actual cell size #481

Merged
merged 11 commits into from Jul 18, 2022

Conversation

ntolley
Copy link
Contributor

@ntolley ntolley commented Jun 20, 2022

Closes #433.

@ntolley
Copy link
Contributor Author

ntolley commented Jun 27, 2022

Going through the code, the issue here is that section['sec_name'].end_pts is not automatically updated when length or other attributes are modified. For example, modifying by net.cell_types['L5_pyramidal'].sections['apical_trunk']._L = 5

In the code however, the length attribute is used by default when building the cell:

# be explicit about letting sec.L dominate over the 3d points used by

In my mind, the easiest solution is to somehow extract the real section.end_pts that Neuron calculates. Ideally without running a whole simulation though and building the whole network.

@rythorpe
Copy link
Contributor

Wouldn't we then need to restrict plotting morphology until after NetworkBuilder is called?

@ntolley
Copy link
Contributor Author

ntolley commented Jun 28, 2022

Wouldn't we then need to restrict plotting morphology until after NetworkBuilder is called?

I was thinking more in line with building the cell in Neuron, updating the values, and then deleting the object. Alternatively, we could make the interface more flexible and explicitly build the whole network without simulating, Then we could allow simulate_dipole to accept either a Network, or a NetworkBuilder object.

@jasmainak
Copy link
Collaborator

You could also copy the logic from here:

https://github.com/neuronsimulator/nrn/blob/f242cc8d4ddefb74f71f7f613a4b7b4043c2b641/src/nrnoc/treeset.cpp#L1233-L1253

Probably easier than rebuilding and destroying neuron objects ... but that trick might be useful elsewhere :)

hnn_core/cell.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jasmainak jasmainak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks clean to me. @rythorpe merge button is yours when ready from @ntolley

@rythorpe
Copy link
Contributor

I think there's still a bit more to do here, like call cell.change_sec_length() whenever a cell's section length attribute is modified. This could be done within Section.

@ntolley
Copy link
Contributor Author

ntolley commented Jul 15, 2022

@rythorpe is right, there's a few things to take care of still. Good suggestion moving it to be a method of Section!

Right now the plots do update. However, all of the neighboring sections remain in place so the pieces are now disconnected. Cell.topology will specify the sections that need to be displaced. If I remember correctly, everything is referenced to the soma, so it should suffice to only move the "child_sec" of the sections being modified (and do so recursively).

@ntolley
Copy link
Contributor Author

ntolley commented Jul 15, 2022

After working at this more, I honestly think building the cell is the best way to actually plot the morphology. Inspecting the geometry more closely, the points defined in Section.end_pts don't even match what's built in Neuron for the default cells.

With a built cell you can compare the two with

print(net.cell_types['L5_pyramidal'].sections['apical_trunk'].end_pts)
print(net.cell_types['L5_pyramidal']._nrn_sections['apical_trunk'].psection()['morphology']['pts3d'])

[[0, 0, 23], [0, 0, 83]]
[(0.0, 0.0, 39.0, 10.199999809265137), (0.0, 0.0, 141.0, 10.199999809265137)]

The length of the apical trunk is 102.0 so the Neuron coordinates are correct (attention that the z axis is in a different spot). I'll see if things start breaking when I create/delete cells in Neuron, but my sense is that this is a lot better than copying a bunch of C code from Neuron and translating it to python.

This also raises the question, why is there such a mismatch with the points defined here?

@ntolley
Copy link
Contributor Author

ntolley commented Jul 15, 2022

Different approach, I've implemented Cell.update_end_pts() so that the object actually stores the real data used in Neuron. Since we're not starting any simulations with _PC I think that del is sufficient to clear the Neuron objects. I can confirm that the morphology plots now accurately reflect the actual cell size:

net = jones_2009_model()
#Default end_pts
net.cell_types['L5_pyramidal'].plot_morphology()

# Actual end_pts
net.cell_types['L5_pyramidal'].update_end_pts()
net.cell_types['L5_pyramidal'].plot_morphology()

# Modified end_pts
net.cell_types['L5_pyramidal'].sections['soma'].L = 500
net.cell_types['L5_pyramidal'].update_end_pts()
net.cell_types['L5_pyramidal'].plot_morphology()

image

@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2022

Codecov Report

Merging #481 (ae49377) into master (c7460ba) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #481      +/-   ##
==========================================
+ Coverage   87.63%   87.74%   +0.10%     
==========================================
  Files          19       19              
  Lines        3792     3826      +34     
==========================================
+ Hits         3323     3357      +34     
  Misses        469      469              
Impacted Files Coverage Δ
hnn_core/cell.py 97.21% <100.00%> (+0.43%) ⬆️

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 c7460ba...ae49377. Read the comment docs.

hnn_core/cell.py Outdated Show resolved Hide resolved
hnn_core/cell.py Outdated Show resolved Hide resolved
@jasmainak
Copy link
Collaborator

I'm not a big fan of the 2-step API ... can you make it so that everything is consistent as soon as you update the lengths? The user shouldn't have to remember to call another method for the lengths to be consistent.

@ntolley
Copy link
Contributor Author

ntolley commented Jul 17, 2022

So I have made it so that all attribute modifications go through the common function Cell.modify_section(). Now every time that function is called, or a cell is created, Cell._update_end_pts() is called.

While before it was nice to be able to directly modify section attributes like Section.Ra and Section.cm, since we're making an API for length and diameter then we may as well have all attributes get changes in the same place.

@jasmainak
Copy link
Collaborator

agreed about that. Consistency in the API is important. Make sure to update whats_new when ready!

@ntolley ntolley changed the title WIP: Make cell morphology plots reflect actual cell size [MRG] Make cell morphology plots reflect actual cell size Jul 17, 2022
doc/whats_new.rst Outdated Show resolved Hide resolved
@jasmainak jasmainak merged commit b66742d into jonescompneurolab:master Jul 18, 2022
@jasmainak
Copy link
Collaborator

Thanks @ntolley ! 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Section.end_pts doesn't update when length is manually is changed
4 participants