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] add method to plot cell morphologies #319

Merged
merged 10 commits into from
Apr 29, 2021

Conversation

jasmainak
Copy link
Collaborator

@jasmainak jasmainak commented Apr 23, 2021

Plots the cell morphology
image

@jasmainak jasmainak changed the title ENH WIP add method to plot cell morphologies [MRG] add method to plot cell morphologies Apr 23, 2021
@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2021

Codecov Report

Merging #319 (caed5ff) into master (eb17311) will increase coverage by 0.10%.
The diff coverage is 95.34%.

❗ Current head caed5ff differs from pull request most recent head 83360c1. Consider uploading reports for the commit 83360c1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   90.43%   90.53%   +0.10%     
==========================================
  Files          14       14              
  Lines        2425     2493      +68     
==========================================
+ Hits         2193     2257      +64     
- Misses        232      236       +4     
Impacted Files Coverage Δ
hnn_core/cell.py 95.45% <ø> (-0.14%) ⬇️
hnn_core/network.py 92.47% <66.66%> (-0.58%) ⬇️
hnn_core/viz.py 87.50% <93.33%> (+1.11%) ⬆️
hnn_core/basket.py 100.00% <100.00%> (ø)
hnn_core/params_default.py 100.00% <100.00%> (ø)
hnn_core/pyramidal.py 98.13% <100.00%> (-0.11%) ⬇️
hnn_core/cell_response.py 83.73% <0.00%> (+2.48%) ⬆️

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 eb17311...83360c1. Read the comment docs.

hnn_core/viz.py Show resolved Hide resolved
hnn_core/params_default.py Show resolved Hide resolved
@@ -16,6 +16,8 @@ Changelog
~~~~~~~~~
- Store all connectivity information under :attr:`~hnn_core.Network.connectivity` before building the network, by `Nick Tolley`_ in `#276 <https://github.com/jonescompneurolab/hnn-core/pull/276>`_

- Add new function :func:`~hnn_core.viz.plot_cell_morphology` to visualize cell morphology in `#319 <https://github.com/jonescompneurolab/hnn-core/pull/319>`_
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Add new function :func:`~hnn_core.viz.plot_cell_morphology` to visualize cell morphology in `#319 <https://github.com/jonescompneurolab/hnn-core/pull/319>`_
- Add new function :func:`~hnn_core.viz.plot_cell_morphology` to visualize cell morphology, by `Mainak Jas`_ in `#319 <https://github.com/jonescompneurolab/hnn-core/pull/319>`_

----------
axes : list of instance of Axes3D
Matplotlib 3D axis
cell_types : 'L5Pyr' | 'L5Basket' | list
Copy link
Contributor

@ntolley ntolley Apr 26, 2021

Choose a reason for hiding this comment

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

Suggested change
cell_types : 'L5Pyr' | 'L5Basket' | list
cell_types : str | list of str

axes : list of instance of Axes3D
Matplotlib 3D axis
cell_types : 'L5Pyr' | 'L5Basket' | list
The cell types.
Copy link
Contributor

@ntolley ntolley Apr 26, 2021

Choose a reason for hiding this comment

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

Suggested change
The cell types.
The cell types. Valid strings include: 'L5Pyr', 'L2Pyr'

hnn_core/viz.py Outdated
----------
axes : list of instance of Axes3D
Matplotlib 3D axis
cell_types : 'L5Pyr' | 'L5Basket' | list
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cell_types : 'L5Pyr' | 'L5Basket' | list
cell_types : str | list of str

hnn_core/viz.py Outdated
axes : list of instance of Axes3D
Matplotlib 3D axis
cell_types : 'L5Pyr' | 'L5Basket' | list
The cell types.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The cell types.
The cell types. Valid strings include: 'L5Pyr', 'L2Pyr'

@cjayb
Copy link
Collaborator

cjayb commented Apr 26, 2021

I like the barebones first pass at (any!) visualisation. This could clearly be improved on in future PRs.

Could we make this 2D? The 3rd dimension is super confusing: rotating the axes isn't stable with effectively two degrees of freedom. As long as our basal oblique dendrites are in the same plane, I think we should plot them in a XZ-system, where Z is the cortical depth direction. The NEURON sections are defined as (x, z, y), which no doubt has a historical reason.

I'd also advocate fixing the Z-range to always start from 0 (white matter begins). That way the size difference between L2/3 and L5 pyramidals becomes obvious, as does their relative depth in cortex.

@jasmainak
Copy link
Collaborator Author

okay starting in reverse order with comments. @cjayb better?

image

it's still a matplotlib 3D plot but since axes are turned off, I don't anticipate users to interact with. It also keeps the door open for future additions where there is a non-zero projection of a dendrite along the third axis

@cjayb
Copy link
Collaborator

cjayb commented Apr 26, 2021

I guess I prefer the new axes-less plot, yes. Emphasises relative size and location, which are important.

def secs(self):
return _secs_Basket()

def set_geometry(self):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this should be unified with Pyramidal class set_geometry but I would keep this for a followup PR

@cjayb
Copy link
Collaborator

cjayb commented Apr 27, 2021

As mentioned in #202, could we please agree on our axes? I think this PR should be relatable to the output of net.plot_cells(). Ultimately they will perhaps merge, but for now, what are the axes labels on each?

The above is clearly rotated to align the apical dendrites with the Z-direction (which intuitively makes more sense to me...)

@jasmainak
Copy link
Collaborator Author

jasmainak commented Apr 28, 2021

@rythorpe see caed5ff which gives

image

now I do concede that the sections are not cylindrical but you don't realize that until you interact with this figure. Personally, for me, this is good enough for the use case I outlined above (educational). Cylindrical is possible although probably not worth it currently

@cjayb
Copy link
Collaborator

cjayb commented Apr 28, 2021

I like this, +1 for merge and iterate later

@jasmainak jasmainak mentioned this pull request Apr 29, 2021
6 tasks
Copy link
Collaborator

@cjayb cjayb left a comment

Choose a reason for hiding this comment

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

I see I hadn't submitted this, sorry... @jasmainak see my one question in the test, otherwise good to merge?

with pytest.raises(ValueError, match='Unrecognized cell type'):
plot_cell_morphology(cell_types='blah')
axes = plot_cell_morphology(cell_types='L2Pyr')
len(axes) == 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing assert?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jasmainak could you confirm this before I merge?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops indeed, fixed now

@cjayb cjayb merged commit 4cbc4b3 into jonescompneurolab:master Apr 29, 2021
@cjayb
Copy link
Collaborator

cjayb commented Apr 29, 2021

Go @jasmainak !

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.

5 participants