Closed mstimberg closed 8 years ago
Nice! Good to go, but just one minor comment. For the masked images, maybe have a grey background rather than white so that you can tell the difference between a point which is masked and one where the colour is white.
I agree that we should have something for plotting multiple variables at the same time and doing things like shared axes. I would postpone this since it needs a bit of discussion, though (creating subplots is not really compatible with the way we allow passing in an axis currently, etc.).
For the masked images, maybe have a grey background rather than white so that you can tell the difference between a point which is masked and one where the colour is white.
Not sure I understand: the masked images are not plotting things in white, that is just the existing background colour (e.g. if you import seaborn
or use the ggplot
theme or something like that then the background will be grey). Do you propose to set the background colour to something else than white? But what if the colormap contains grey?
Yep, happy to postpone that. Will create an issue for it.
For the background, yes I just mean that the background of the plot should be a different colour. What about a hatched background, something like this but prettier (more subtle hatching colours)?
http://stackoverflow.com/questions/18390068/hatch-a-nan-region-in-a-contourplot-in-matplotlib
I have to say I am not that convinced... When the default background color does not clash with the colormap (as with the default matplotlib settings), then I think it looks best without any hatching. Isn't this the kind of thing we want do leave up to the user? If they manually set a colormap that includes the color of the background, then they can also manually set the background color, no?
I agree it should all be optional, but as a default for brian_plot
it could be nice to have it. Here's a version that I think looks good: https://gist.github.com/thesamovar/15f0e962ca55530f5bd01d9a4626365d
It's how photoshop and other image editors handle transparent points and I think it works well.
I agree it looks good but I am still hesitant for the following reasons:
interpolation
for image plots or edgecolor
for scatter plots), you can overwrite this change by providing a keyword argument to brian_plot
or plot_synapses
-- it is not quite the same for the hatching here.I think it is a somewhat fundamental choice -- we could tweak brian_plot
to always create plots with nice defaults (line-widths, colors, ...) and only leave plot_synapses
etc. at the defaults. However, for some plots (e.g. if you recorded multiple variables) you do not have the choice to use brian_plot
, and then you'd switch from "nice plot" to "basic plot" which is not great.
One more concern: according to the documentation "Hatching is supported in the PostScript, PDF, SVG and Agg backends only." I am not sure whether non-Agg backends are used anywhere anymore and what happens if you try to use hatching with them (a warning would be ok but an error would not).
Another idea, going away slightly from the simple call-and-forget approach for brian_plot
but potentially also useful in other contexts: have a separate function add_background_pattern
:
ax = brian_plot(synapses)
add_background_pattern(ax)
?
OK, you've convinced me!
If the defaults for matplotlib don't have this problem I think it's fine. I just noticed on some of the examples in the docs you had white for both a non-existing value and the highest value, which is confusing.
I think I might add it as an option to plot_synapses
? Or just leaving it out is fine too actually.
In which case, I think we're good to merge.
I just noticed on some of the examples in the docs you had white for both a non-existing value and the highest value, which is confusing.
Ah, indeed -- this is an example where I show how to use plot_synapses
with custom settings (including the colormap), I'll change it so that it also sets the background color.
I think I might add it as an option to plot_synapses? Or just leaving it out is fine too actually.
What do you think of the add_background_pattern
option? We could have it use your values (hatch='+++', fill=True, fc=(0.9,)*3, ec=(0.8,)*3, zorder=-10
) as default arguments.
Yep, that works too!
Actually I might change to 'xxx' rather than '+++' because the '+' makes it look like the periodicity of the pattern should be the same as the number of pixels, which isn't true.
Actually I might change to 'xxx' rather than '+++' because the '+' makes it look like the periodicity of the pattern should be the same as the number of pixels, which isn't true.
Ok, I'll have a go at it (what do you think of the name, maybe 'add_bg_pattern` instead?) and use it in one of the example plots.
I don't have very strong feelings on the name. I think I slightly prefer background
to bg
. Maybe add_hatching
, add_background_hatching
or add_background_pattern
.
Or just hatch_background
?
I think I'd stay with add_background_pattern
. I don't feel hatch
/hatching
is a very well-known term, or is it?
Yeah pattern is probably more well known, indeed.
Alright, that should be it.
Great! I added two annoying comments. ;)
By the way, the new (to me at least) image diffs in github are great! Have you tried playing around with it?
By the way, the new (to me at least) image diffs in github are great! Have you tried playing around with it?
Uh, no I did not actually see that you now have those fancy swipe and onion skin(!) options. Pretty neat!
So, pending the test suite this is ok to merge, right? After the merge, I'll do a new release right away, it would be good to fix the ugly transposed matrix issue as soon as possible.
Yep!
This fixes #6 and generally cleans up a bit the synaptic plotting (consistent colors, masked arrays so that non-existing connections are not shown as zeros but not shown at all). It also introduces the new feature that you can directly use e.g.
brian_plot(syn.delay)
.If you are fine with the changes then I think I'll release 0.1.2 after merging.