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

[BUG] Size of implant's electrode don't seem to agree with the provided electrode radius #340

Closed
alex-bene opened this issue Jan 20, 2021 · 5 comments
Milestone

Comments

@alex-bene
Copy link

Bug Description
Trying to visualize various custom parametrized square implants, it seems like the 'r' parameter does not agree with the radius of the electrode when the implant is visualized using plot_implant_on_axon_map.

To Reproduce
plot_implant_on_axon_map(ProsthesisSystem(ElectrodeGrid((2, 2), r=1000, spacing=(2000), etype=DiskElectrode)))

Expected behavior
In the diagram from the above snippet, there should be 4 electrodes with a radius of 1000 microns touching each other and covering a total area of 4000x4000 microns.

Screenshots
However we get this
image

Why this is happening
Looking around the code this seems to trackback to this line of code where the markersize is set

markersize=np.sqrt(implant[e].r) * 2)

In the End
In the end, what matter is if it is just a visual thing and you couldn't find how to better visualize it (because I looked into it and I didn't find something very useful except that the units of markersize are points^2 --> area) or if the 'r' in the code snippet above means something different than the radius of the electrode in microns (since this would have impact on the results from the simulations)

@alex-bene alex-bene added the bug label Jan 20, 2021
@mbeyeler
Copy link
Member

Thank you for this detailed report. Yeah, as you pointed out towards the end, it is a pain to get the marker to show up with the correct size because of the auto-scaling... I realize though that it is important to get this right - I will look into it

@alex-bene
Copy link
Author

In my case, it just happened to be using the visualization to check for electrode overlaps so it was a bit jarring. In any case, I suppose if it was documented that the visual size of the electrode is not the real one would be a good start.

As for a possible solution, I suppose one could use the circle object from matplotlib.

ax.add_patch(plt.Circle((implant[e].x, implant[e].y), implant[e].r, color='w'))

As for using the 'markersize' option, according to kite is seems like:

Using matplotlib.pyplot.plot() determines the width of points. Using matplotlib.pyplot.scatter() determines the area of points
source

In any case, it seems like:
actual_area = markersize*k in the case of scatter
actual_diameter = markersize*k in the case of plot
where k could be calculated through some measurements or trial and error maybe. (although I strongly believe that the use of plt.Circle is the best)

I will try to make some of the above modifications probably in the next week if I find some time.

@mbeyeler
Copy link
Member

A PR would be greatly appreciated! Then we can ship this as part of v0.7, which is imminent (~next couple weeks)

@mbeyeler mbeyeler added this to the v0.7 milestone Jan 26, 2021
@mbeyeler
Copy link
Member

mbeyeler commented Feb 12, 2021

Looking further into this: The use of these plotters should actually be deprecated. This is a documentation fail.

The new preferred way is to use the plot methods of the corresponding object - those correctly use Circle.

For example, plot Argus II:

import pulse2percept as p2p
p2p.implants.ArgusII().plot()

image

Plot Argus II on axon map:

p2p.models.AxonMapModel().plot()
p2p.implants.ArgusII().plot()

image

Or use your own implant:

p2p.implants.ElectrodeGrid((2, 2), r=1000, spacing=2000, etype=p2p.implants.DiskElectrode).plot()

image

On axon map:

p2p.models.AxonMapModel().plot()
p2p.implants.ElectrodeGrid((2, 2), r=1000, spacing=2000, etype=p2p.implants.DiskElectrode).plot()

image

@alex-bene
Copy link
Author

Ohh, I actually didn't test that. It seems you're right. Well as a "plus" I suppose the same functionality could be added to the "plot_implant_on_axon_map" function for consistency or completely remove it.

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

No branches or pull requests

2 participants