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

WIP: Upgrade plot_morphology #499

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mjpelah
Copy link
Contributor

@mjpelah mjpelah commented Jul 12, 2022

Closes #496

Goals:

  • Create new viz.py and corresponding net.py method to plot morphologies of the cells within a network: plot_cell_morphologies
  • Move common functionality of plotting cell morphologies in plot_cell_morphology and (new) plot_cell_morphologies to private method in viz.py: _plot_cell

So far, the three new methods are setup to discuss code architecture, but not complete.

@jasmainak
Copy link
Collaborator

Looks good so far! Let's see how it shapes when you have it working

hnn_core/viz.py Outdated

parameters
----------
cell_type : instance of net.cell_type[]
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't really specify the object being passed to the function since net.cell_types is a dictionary:

cell_types : dict

Is the goal to just pass the cell name, or the cell object to the function? If it's the latter, this would be an instance of the Cell class:

class Cell:

@ntolley ntolley changed the title Upgrade plot_morphology WIP: Upgrade plot_morphology Jul 13, 2022
@ntolley
Copy link
Contributor

ntolley commented Jul 13, 2022

Looks good so far! Let's see how it shapes when you have it working

Agreed! Also @mjpelah I added the tag "WIP" before the title to indicate the PR is still a work in progress.

@rythorpe
Copy link
Contributor

One idea I pitched to @mjpelah was to allow viz.plot_cell_morphology to accept a Cell or list of Cell as it's first argument so that Cell.plot_morphology and Network.plot_cell_morphology are both wrappers for the same viz.py function. This would maximize the flexibility of viz.plot_cell_morphology so that users can plot an arbitrary number of cells on the same axis if they wish (myself being a user who might wish to do this). Any thoughts on or objections to this?

@jasmainak
Copy link
Collaborator

I like that! Would you have another argument that specifies the soma locations?

hnn_core/viz.py Outdated

for cell in net.cell_types.values():
ax = _plot_cell(cell, ax=ax, plt=plt, color=colors[i], show=True)
i+=1
Copy link
Collaborator

Choose a reason for hiding this comment

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

indexing is for Matlab and C++ folks ;-) enumerate is more pythonic

@mjpelah
Copy link
Contributor Author

mjpelah commented Jul 15, 2022

Current iteration has three functions in viz.py:

  • plot_cell_morphology
  • plot_cell_morphologies
  • _plot_cell

Output for plot_cell_morphologies is:

image

As you can see, the cells are not offset (next step) as only the L2_pyramidal and L2_basket show, with the others (presumably) behind.

I thought I would try this implementation however, after thinking it through with Ryan, I believe it would be best to have one just one function that takes either a single cell or a list (or dictionary), creates a list (if only a single cell was passed in) and iterates through the list, plotting each cell.

Thoughts?

@@ -1271,6 +1271,25 @@ def plot_cells(self, ax=None, show=True):
"""
return plot_cells(net=self, ax=ax, show=show)

def plot_cell_morphologies(self, ax=None, show=True):
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you share a plot or add it to an example once it's ready? so we can see how it looks

@jasmainak
Copy link
Collaborator

I think it's worth thinking through how the user would use the function. If you have a single function for one cell and multiple cells, how would the offsets be plotted? Would you still plot the cell with the same z offset if it was the only cell being plotted?

hnn_core/viz.py Outdated
plt.figure()
ax = plt.axes(projection='3d')

colors = ['b', 'c', 'r', 'm']
Copy link
Contributor

Choose a reason for hiding this comment

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

I would set the colormap (e.g., 'cividis') as a default arg when calling the function.

@rythorpe
Copy link
Contributor

I think it's worth thinking through how the user would use the function. If you have a single function for one cell and multiple cells, how would the offsets be plotted? Would you still plot the cell with the same z offset if it was the only cell being plotted?

I think the best way to handle this would be to get the morphological radius for each cell and then plot each successive cell at a distance equal to the sum of the radii for each pair of adjacent cells. Then, the user could input a Cell instance or list of Cell instances without having to worry about overlapping on the same axis.

@jasmainak
Copy link
Collaborator

We also need the z-axis to be visible !

@mjpelah
Copy link
Contributor Author

mjpelah commented Jul 16, 2022

I think it's worth thinking through how the user would use the function. If you have a single function for one cell and multiple cells, how would the offsets be plotted? Would you still plot the cell with the same z offset if it was the only cell being plotted?

I think the best way to handle this would be to get the morphological radius for each cell and then plot each successive cell at a distance equal to the sum of the radii for each pair of adjacent cells. Then, the user could input a Cell instance or list of Cell instances without having to worry about overlapping on the same axis.

The solution @rythorpe proposed might allow for more readable code. If you used separate functions for a cell vs cells, you would then need a private function (as in the most recent commit) because they're would be a lot shared (redundant) code between the two. This would be fine, calling the private function once for a cell and more than once (for loop) for cells, but how would you set the offset for multiple cells?

You could:

  • Individually pass in the cell position when dealing with multiple cells, but might increase amount of redundant code
  • Pass in the number of cells you are plotting as an int, therefore it could create the varying positions for the offset itself: this could either be done by recursively calling _plot_cell within the function or with a loop.
  • Perhaps set, only, the cell position offset in plot_cell_morphologies, and have it carry over to _plot_cell, however I do not know how this would be done or if it is possible

I feel like with any of these implementations, it would be difficult to read or there would be redundant code. Alternatively, to have one function that treats any cell (or cells) that are passed in, as a list of cells, and iterate through the list setting a new offset for each subsequent iteration, might be less redundant.

@mjpelah
Copy link
Contributor Author

mjpelah commented Jul 16, 2022

We also need the z-axis to be visible !

@jasmainak z-axis as well as x and y?

I thought we were first going to get something like this, without z axis:
image

@jasmainak
Copy link
Collaborator

actually is it possible to share axis between a 3D plot and a 2D plot? can you try plotting this alongside the LFP to see how it looks?

@jasmainak
Copy link
Collaborator

but that plot shows the z axis ...

@jasmainak
Copy link
Collaborator

I was thinking of two separate functions that call a common private function. In each function you pass the position ... when it's only one cell, the position is z=0, when it is called from the method of network to plot all the cell types, use the positions of the cells in the network.

@mjpelah
Copy link
Contributor Author

mjpelah commented Jul 16, 2022

but that plot shows the z axis ...

Oh yes, my mistake. I'm used to x being left right, y being up down, and z being forward back.

@jasmainak
Copy link
Collaborator

Try to first plot it alongside LFP with 2 subplots and see if it makes sense. Then we can make the code organization nice :)

@ntolley
Copy link
Contributor

ntolley commented Jul 16, 2022

Small nitpick, the _plot_cell() function is a bit confusingly named as there is a plot_cells() function which is called from the Network and plots the soma positions.

Perhaps the next PR could merge this functionality and rename plot_cells() to plot_somas() or something. Then we could have an option to plot the full network with real cell morphologies as well (although it is extremely slow, trust me I checked 😄)

@jasmainak
Copy link
Collaborator

I like plot_somas !

@jasmainak
Copy link
Collaborator

any updates here?

@mjpelah
Copy link
Contributor Author

mjpelah commented Jul 18, 2022

any updates here?

Nothing significant. I've got a poster presentation to create for Monday on the LFP/CSD project so that is taking up most of my time

@ntolley
Copy link
Contributor

ntolley commented Jul 19, 2022

This can be saved for another PR, but two things that would be really nice to have in plot_cell_morhology is the ability to specify different colors for each section, as well as specify the position of the cell (current the cell is moved to the (0,0,0) coordinate automatically).

To see an example of how this could be implemented you can check out the changes made here.

@mjpelah
Copy link
Contributor Author

mjpelah commented Jul 20, 2022

Just an update on progress:

  • All cell morphologies are now handled by one function
  • If a single cell or dict or cells is input, they are placed in a list
  • The list is iterated, plotting each

For multiple cells, an offset based on the radius of the cell is created, however is not completely working yet:
Test_Figure_2
Test_Figure_2_angle2

@jasmainak
Copy link
Collaborator

Something went wrong with your git ...

And you need to make this a 2D plot or have an option for 2D plot, otherwise I don't know if you can combine it with the LFP plot.

@rythorpe
Copy link
Contributor

Yeah, something is off. It almost looks like you rebased master onto your upgrade_plot_morphology branch instead of vice versa.

@jasmainak I had advised Matt to get the logic down first, as there are a few options we discussed yester of how to place the figure into a subplot so that it looks pretty and is visually aligned with the y-axis of an LFP plot.

@jasmainak
Copy link
Collaborator

okay sounds good. I just want to make sure you are thinking of that goal. Because the logic and code structure might be slightly different if you have only 2 dimensions to plot the morphology

@jasmainak
Copy link
Collaborator

@mjpelah looks like you need to rebase :) :) :)

@mjpelah
Copy link
Contributor Author

mjpelah commented Aug 9, 2022

@mjpelah looks like you need to rebase :) :) :)

@jasmainak Yep, just got home, have been busy with all the symposiums and presentations; will rebase and continue tomorrow!

@jasmainak
Copy link
Collaborator

sounds good, thank @mjpelah

@mjpelah
Copy link
Contributor Author

mjpelah commented Aug 10, 2022

@jasmainak could we meet very briefly (5-10m) to discuss some things with this PR and some issues I had with git? It would be a big help!

@jasmainak
Copy link
Collaborator

Sorry I just saw this email. Can you shoot me an email with a zoom id? I can join anytime now

@rythorpe
Copy link
Contributor

How's it going @mjpelah? Anything we can assist with here?

@mjpelah
Copy link
Contributor Author

mjpelah commented Oct 26, 2022

How's it going @mjpelah? Anything we can assist with here?

Hey Ryan, thanks for checking in. Haven't had time to make progress, will update as soon as I’ve got something.

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.

upgrade plot_morphology
4 participants