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

upgrade plot_morphology #496

Open
jasmainak opened this issue Jul 8, 2022 · 9 comments · May be fixed by #499
Open

upgrade plot_morphology #496

jasmainak opened this issue Jul 8, 2022 · 9 comments · May be fixed by #499
Milestone

Comments

@jasmainak
Copy link
Collaborator

Currently plot_morphology is a method of Cell. However, for LFP plotting it is useful to have it as a method of Network. The current code would be refactored into a private function that will be called from the new plot_cells method? The code will iterate of the cells in cell_type and create a plot whose y-axis is the same as the LFP/CSD plot.

@mjpelah do you want to give this a shot?

@mjpelah
Copy link
Contributor

mjpelah commented Jul 8, 2022

@jasmainak Sure thing!

@mjpelah
Copy link
Contributor

mjpelah commented Jul 11, 2022

There is:
plot_morphology in Cell
plot_cell_morphology in Viz
plot_cells in Network
plot_cells in Viz

The plotting is being done in Viz as the methods plot_cells and plot_cell_morphology, with Network and Cell each having their own corresponding accessor methods.

So currently, plot_cells is redundantly repeating code that could be performed by plot_cell_morphology, however, plot_cell_morphology is too specific to be used for this process.

So the goal is to take the general 'cell plotting' code in plot_cell_morphology and plot_cells, and move it to a new private method (ex. _plot_cell), therein calling this new private method in both plot_cell_morphology and plot_cells.

@jasmainak Do I have that right?

Additionally, the plot_cells method has the same name in Network as it does in Viz, however, plot_morphology in Cell has a different name than it does in Viz: plot_cell_morphology. Wouldn't it be best to standardize this among the pairs? If so, do you prefer they have the same name or different?

@jasmainak
Copy link
Collaborator Author

So currently, plot_cells is redundantly repeating code that could be performed by plot_cell_morphology

nopes, they are different visualizations:

image

what I am imagining is something like this:

image

or

image

as in this paper: https://www.sciencedirect.com/science/article/pii/S1053811920309526

@jasmainak
Copy link
Collaborator Author

jasmainak commented Jul 11, 2022

Btw, all the visualization functions are put in one file so that the code is all in one place and redundancy is avoided between functions. Then they are called from the methods of the classes. Sometimes it's nice to have a different name. For example if you a plotting method for Dipole, then you want to do:

dipole.plot()

but in viz.py it's better to have more specific names because there might be several plotting functions and all of them cannot be called plot. You could for e.g., call it plot_dipole in viz.py

@mjpelah
Copy link
Contributor

mjpelah commented Jul 11, 2022

Ahh yes I understand.

So you want a new method (ex. _plot_cell_type_morphologies), which calls plot_cell_morphology, that iterates through the cell types and creates models for each one. This new method would then be called in plot_cells to create the cell model visualization alongside the network visualization.

@jasmainak is that right?

@jasmainak
Copy link
Collaborator Author

jasmainak commented Jul 11, 2022

I would create a separate function/method. You can always combine it with other visualizations by creating subplots. E.g.:

fig, axes = plt.subplots(2, 1)
net.plot_morphology(ax=axes[0])
net.plot_cells(ax=axes[1])

I'm sorry I had forgotten that a plot_cells method already existed. Now, move the common logic between net.plot_morphology and cell.plot_morphology to a private function so that there is minimum repetition in the code.

@mjpelah
Copy link
Contributor

mjpelah commented Jul 11, 2022

Btw, all the visualization functions are put in one file so that the code is all in one place and redundancy is avoided between functions. Then they are called from the methods of the classes. Sometimes it's nice to have a different name. For example if you a plotting method for Dipole, then you want to do:

dipole.plot()

but in viz.py it's better to have more specific names because there might be several plotting functions and all of them cannot be called plot. You could for e.g., call it plot_dipole in viz.py

This I understand now, makes perfect sense.

@mjpelah
Copy link
Contributor

mjpelah commented Jul 11, 2022

I would create a separate function/method. You can always combine it with other visualizations by creating subplots. E.g.:

fig, axes = plt.subplots(2, 1)
net.plot_morphology(ax=axes[0])
net.plot_cells(ax=axes[1])

Sounds good!

I'm sorry I had forgotten that a plot_cells method already existed. Now, move the common logic between net.plot_morphology and cell.plot_morphology to a private function so that there is minimum repetition in the code.

I thought plot_cells plotted the network visualization? In addition, there is no such net.plot_morphology function, in my version of hnn-core.

My understanding was that you wanted me to create a plot_cells_morphology function, and move the common logic between plot_cells_morphology and plot_cell_morphology to a shared private function.

I think I have a pretty good understanding of what needs to be done. If it's alright with you, let me give it a shot, and we can move this conversation to a PR @jasmainak .

@jasmainak
Copy link
Collaborator Author

I thought plot_cells plotted the network visualization?

yes. I mean that I confused you in the original post by using that name since that function is already for a different purpose.

Give it a shot and we can discuss more there!

@mjpelah mjpelah linked a pull request Jul 12, 2022 that will close this issue
@ntolley ntolley added this to the 0.5 milestone Jul 10, 2024
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 a pull request may close this issue.

3 participants