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.

Sign up for GitHub

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

Jump to bottom

allows empty rasters #601

Merged
merged 4 commits into from
Jan 22, 2015
Merged

allows empty rasters #601

merged 4 commits into from
Jan 22, 2015

Conversation

celiasmith
Copy link
Contributor

this is useful for generating animations

@Seanny123
Copy link
Contributor

LGTM

@Seanny123 Seanny123 added this to the 2.0.1 release milestone Dec 26, 2014
hunse
hunse reviewed Dec 29, 2014
View reviewed changes
nengo/utils/matplotlib.py
@@ -89,7 +89,8 @@ def rasterplot(time, spikes, ax=None, **kwargs):
ax.set_ylim(len(spikes) - 0.5, -0.5)
if len(spikes) == 1:
ax.set_ylim(0.4, 1.6) # eventplot plots different for len==1
ax.set_xlim(left=0, right=max(time))
if time.any():
ax.set_xlim(left=0, right=max(time))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm worried that this function assumes that time begins at zero.

@hunse hunse force-pushed the rasterplot-fixes branch 2 times, most recently from 52b990b to 485e4a2 Compare December 30, 2014 23:12
@hunse
Copy link
Collaborator

hunse commented Dec 30, 2014

I've pushed an additional commit that cleans up rasterplot a bit.

@Seanny123
Copy link
Contributor

LGTM, again. 🌴

@tbekolay
Copy link
Member

tbekolay commented Jan 8, 2015

This looks great! I'm curious if we shouldn't bother with eventplot? My reasoning here is that running test_rasterplot generates a 15KB PDF without eventplot, and 3.1MB with eventplot. My understanding of why this happens is because plot uses blitting, while eventplot does not.

Alternatively, we could use eventplot but rasterize it, which reduces the PDF size to 84KB, but (since it's rasterized) doesn't look as nice as the plot version when you zoom in closely.

@hunse
Copy link
Collaborator

hunse commented Jan 8, 2015

The difficulty with plot is choosing a good marker size. This isn't too big of a problem if you know your figure size and number of neurons in advance (such as when making figures for a paper), since you can just play around with the marker size until it looks good. The problem occurs when you want to be able to resize the figure, since there's no way to tie the marker size to the axes (data), so for example if you make the figure larger then the markers will be too small by comparison.

But I didn't realize that the difference in file sizes was so large, and taking out eventplot would make the code easier. Another option is to turn eventplot off by default, but still have it there if people really want it.

@tbekolay
Copy link
Member

tbekolay commented Jan 9, 2015

I like the idea of plot by default, and eventplot still available if desired. I'll try implementing a heuristic marker size based on the figure size and the number of neurons at the point when rasterplot is called, but yeah, if people change the figure size after the fact we can't do much (though, it is possible to implement a figure resize handler, it'd just be Matplotlib backend-specific).

@hunse
Copy link
Collaborator

hunse commented Jan 16, 2015

I added a heuristic for the marker size that seems to work pretty well.

@tbekolay
Copy link
Member

Great! Let me test it out a bit then I'll merge.

@tbekolay
Copy link
Member

Added some commits; take a look @hunse and if it looks good I'll merge.

@hunse
Copy link
Collaborator

hunse commented Jan 21, 2015

LGTM, but this is getting tripped up on the Mock plot objects used to skip plotting in the examples.

@tbekolay
Copy link
Member

Oh right! Forgot about that. Will fix tomorrow.

@tbekolay
Copy link
Member

Hacked together a fix in c9ae1ef. Look good @hunse or too much of a hack?

@hunse
Copy link
Collaborator

hunse commented Jan 21, 2015

I was trying to think if there's a decent comment you could put there to explain it, but I can't think of anything.

I think that hack will be fine for the time being. It would be nicer if we could somehow completely skip things like rasterplot when we're using the mock plotter, but I don't know an easy way to do that, so we can just patch these kinds of problems individually as they crop up.

celiasmith and others added 3 commits January 22, 2015 10:34
@celiasmith @tbekolay
Allow empty rasters in rasterplot
e94cf39 
This is useful for generating animations.
@hunse @tbekolay
Improvements to rasterplot
1fc3966 
- Made the rasters with and without `eventplot` look more similar.
- Gave users the option to choose whether `eventplot` is used.
  This is useful if you want to enforce consistency across machines,
  for example by forcing `rasterplot` to not use `eventplot` so that
  the plot looks the same no matter the Matplotlib version.
- Added an optional test/benchmark for `rasterplot` that just makes
  plots both with and without `eventplot` for easy assessment.
@hunse @tbekolay
Added heuristic for rasterplot marker size
42c3a53 
The default markersize is based on the size of the axes divided
by the number of neurons. This heuristic works well as long as
the user does not resize the plot (there's really nothing we can
do in that case except use `plt.eventplot`, which has performance
issues). We default to using `plt.plot` and the heuristic,
but users can still specify to use `plt.eventplot` if they want.

Using plot closely matches eventplot for all values of `n_neurons`,
so this is used by default now for performance reasons
(PDFs generated by `eventplot` can get very large).

Other changes:
- Implemented a simple `Plotter.Mock.__mul__` method to avoid an
  error in `test_izhikevich` when `NENGO_TEST_PLOT != 1`
@tbekolay
Add changelog entries
db48599 
For this PR and #627, which didn't have a changelog entry added.
@tbekolay tbekolay merged commit db48599 into master Jan 22, 2015
@tbekolay tbekolay deleted the rasterplot-fixes branch January 22, 2015 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
2.0.1 release
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@celiasmith @Seanny123 @hunse @tbekolay