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

Add a new widget to show the units topological distribution on the probe #3142

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

FrancescoNegri
Copy link

@FrancescoNegri FrancescoNegri commented Jul 4, 2024

This contribution aims at introducing a novel widget displaying the spatial distribution of sorted units with respect to the probe layout.
The supported visualizations are:

  1. Unidimensional histogram (along the depth axis)
  2. Bidimensional histogram
  3. Bidimensional kernel density estimates (KDE)

image

@alejoe91 alejoe91 added the widgets Related to widgets module label Jul 4, 2024
Copy link
Member

@alejoe91 alejoe91 left a comment

Choose a reason for hiding this comment

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

Thanks @FrancescoNegri

See my comments. Can you also enable maintainers to push to your PR so it gets automatically formatted?

src/spikeinterface/widgets/widget_list.py Outdated Show resolved Hide resolved
src/spikeinterface/widgets/widget_list.py Outdated Show resolved Hide resolved
src/spikeinterface/widgets/widget_list.py Outdated Show resolved Hide resolved
src/spikeinterface/widgets/widget_list.py Outdated Show resolved Hide resolved
@yger
Copy link
Collaborator

yger commented Jul 8, 2024

Cool, looks very nice! Is there a particular reason why the histogram would be restricted to 1D only? Could we have it also 2D, for planar MEA?

@edit: it seems that this is already handling 2D, I only judged based on the plots, sorry for that

@FrancescoNegri
Copy link
Author

FrancescoNegri commented Jul 9, 2024

Thanks @FrancescoNegri

See my comments. Can you also enable maintainers to push to your PR so it gets automatically formatted?

Yeah, I can see the comments and I will fix the imports.
I am trying to enable you to push to this PR, but I cannot find the check box 😅

@edit: the issue might be that the PR comes from an organization

@FrancescoNegri
Copy link
Author

FrancescoNegri commented Jul 9, 2024

Cool, looks very nice! Is there a particular reason why the histogram would be restricted to 1D only? Could we have it also 2D, for planar MEA?

@edit: it seems that this is already handling 2D, I only judged based on the plots, sorry for that

I considered the UnitDepthsWidget class as reference, thus with the idea of a depth_axis. However, we can make that parameter optional and if not specified the function can assume planar 2D MEAs.

It would be interesting to explore also the possibility to extend this kind of representation to 3D array matrices.

@alejoe91
Copy link
Member

alejoe91 commented Jul 9, 2024

Can you then run pre-commit on your end?

@alejoe91
Copy link
Member

alejoe91 commented Jul 9, 2024

Thanks @FrancescoNegri, can you add a simple test in the tests/test_widgets.py? Even just running the function with a few params changes is fine

Comment on lines 4 to 6
from probeinterface import Probe
from probeinterface.plotting import get_auto_lims
from seaborn import color_palette
Copy link
Member

Choose a reason for hiding this comment

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

you should move these to the plot_matplotlib function. Also seaborn is not installed by default. Could you use a matplotlib palette?

Copy link
Author

@FrancescoNegri FrancescoNegri Jul 9, 2024

Choose a reason for hiding this comment

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

I can definitely use a matplotlib palette, but I was thinking to add an optional seaborn import (if available) to enable also seaborn palettes. I used seaborn also in other parts of the function, I will try to fix it, though I am not sure I will be able to easily plot kernel desnity estimates with matplotlib only.

Is there a specific reason to move probeinterface imports to the plot_matplotlib function? I use its get_auto_lims function to compute xrange and yrange, that are independent on the visualization backend.

Copy link
Member

Choose a reason for hiding this comment

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

yes that souns good! The reason are the failing tests ;)

Core tests only install minimal dependencies. Upon collecting tests across modules, if something is not installed it'll throw an error.

Copy link
Author

Choose a reason for hiding this comment

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

I checked the core tests. It seems that probeinterface is installed, but matplotlib is not, thus probeinterface.plotting cannot be imported. Is that right?

@FrancescoNegri
Copy link
Author

I spent some time trying to replace all seaborn dependencies with matplotlib ones. The issue is that it is not as much straightforward as I thought and some functionalities of the UnitSpatialDistributionsWidget class will be probably dropped.

I have seen that there are other classes relying on seaborn such as the StudyPerformances widget. Wouldn't be worth it to add seaborn to the dependencies of SpikeInterface? Since matplotlib is already there, it would come with minimal overhead.

@alejoe91
Copy link
Member

alejoe91 commented Jul 9, 2024

Thanks for trying! I'm ok in adding it to the "widgets" installation :)

@FrancescoNegri
Copy link
Author

FrancescoNegri commented Jul 10, 2024

Just a quick check. I was going through the probeinterface code. The get_auto_lims function defined in the plotting namespace does not use matplotlib and it could be useful in other contexts where matplotlib might not be available (for instance when estimating the probe limits for different plotting backends).
Wouldn't be better to move it to the utils namespace then? The get_auto_lims function relies exclusively on vanilla Python and numpy.
In case you agree, I can open a pull request with those changes in the probeinterface repo :)

@alejoe91
Copy link
Member

That sounds good! 😊

@alejoe91
Copy link
Member

@FrancescoNegri is this ready for review?

@alejoe91 alejoe91 added this to the 0.101.1 milestone Aug 27, 2024
@FrancescoNegri
Copy link
Author

@alejoe91 not really, I've been off the whole month of August. I will be back to work in a week. Hopefully, I should push a ready-for-review version by mid September.

@alejoe91
Copy link
Member

@alejoe91 not really, I've been off the whole month of August. I will be back to work in a week. Hopefully, I should push a ready-for-review version by mid September.

No rush! just checking in!

@alejoe91 alejoe91 removed this from the 0.101.1 milestone Aug 27, 2024
@FrancescoNegri
Copy link
Author

I updated all of the code and is now ready to be pushed, though I am having an issue and I would like to discuss it with you.
The widget is configured such that it can show:

  1. Only the "joint" plot, thus the histogram/kde on the probe shape.
  2. The "joint" plot and 1 "marginal" plot, If the probe has a preferential direction (depth_axis parameter) I am likely only interested in a 1D marginal histogram along that direction.
  3. The "joint" plot and 2 "marginal" plots. This is useful when we deal with 2D MEAs and we might want to visualize the marginal histograms on both x and y axes.

Given this starting point, I am trying to automatically determine the proportions of the "joint" and "marginal" plot(s) given the default or user-specified figsize and adding the "marginal" axes to the "joint" one by using an AxisDivider. However, given the huge diversity in probe shapes, I am not finding an optimal solution.

Do you by chance have suggestions, ideas or comments about this? @alejoe91 @yger
Thank you in advance! :)

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

Successfully merging this pull request may close these issues.

3 participants