hnn-core icon indicating copy to clipboard operation
hnn-core copied to clipboard

WIP: Upgrade plot_morphology

Open mjpelah opened this issue 2 years ago • 31 comments

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.

mjpelah avatar Jul 12 '22 19:07 mjpelah

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

jasmainak avatar Jul 12 '22 19:07 jasmainak

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.

ntolley avatar Jul 13 '22 09:07 ntolley

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?

rythorpe avatar Jul 13 '22 22:07 rythorpe

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

jasmainak avatar Jul 13 '22 22:07 jasmainak

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?

mjpelah avatar Jul 15 '22 20:07 mjpelah

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?

jasmainak avatar Jul 15 '22 20:07 jasmainak

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.

rythorpe avatar Jul 15 '22 20:07 rythorpe

We also need the z-axis to be visible !

jasmainak avatar Jul 16 '22 00:07 jasmainak

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 avatar Jul 16 '22 00:07 mjpelah

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

mjpelah avatar Jul 16 '22 00:07 mjpelah

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 avatar Jul 16 '22 00:07 jasmainak

but that plot shows the z axis ...

jasmainak avatar Jul 16 '22 00:07 jasmainak

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.

jasmainak avatar Jul 16 '22 00:07 jasmainak

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.

mjpelah avatar Jul 16 '22 00:07 mjpelah

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

jasmainak avatar Jul 16 '22 00:07 jasmainak

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 :smile:)

ntolley avatar Jul 16 '22 14:07 ntolley

I like plot_somas !

jasmainak avatar Jul 16 '22 14:07 jasmainak

any updates here?

jasmainak avatar Jul 18 '22 12:07 jasmainak

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

mjpelah avatar Jul 18 '22 16:07 mjpelah

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.

ntolley avatar Jul 19 '22 12:07 ntolley

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

mjpelah avatar Jul 20 '22 18:07 mjpelah

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.

jasmainak avatar Jul 20 '22 18:07 jasmainak

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.

rythorpe avatar Jul 20 '22 19:07 rythorpe

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 avatar Jul 20 '22 19:07 jasmainak

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

jasmainak avatar Aug 09 '22 01:08 jasmainak

@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!

mjpelah avatar Aug 09 '22 02:08 mjpelah

sounds good, thank @mjpelah

jasmainak avatar Aug 09 '22 17:08 jasmainak

@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!

mjpelah avatar Aug 10 '22 16:08 mjpelah

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

jasmainak avatar Aug 10 '22 19:08 jasmainak

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

rythorpe avatar Oct 25 '22 20:10 rythorpe