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

marker size change #1121

Merged
merged 1 commit into from
Jul 29, 2016
Merged

marker size change #1121

merged 1 commit into from
Jul 29, 2016

Conversation

celiasmith
Copy link
Contributor

@celiasmith celiasmith commented Jul 13, 2016

Description:
Changes the size of markers on the spike rasters when plotting with rasterplot

Motivation and context:
Rasters are usually plotted without overlap of spikes from different neurons, this makes that the case for Nengo.

How has this been tested?
Ran raster plots for papers across a few models. Only on Mac OSX.

Where should a reviewer start?
It's one line. I'd start there. 🍶

How long should this take to review?
10 seconds

Types of changes:
?

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly.
  • I have included a changelog entry.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@mention-bot
Copy link

@celiasmith, thanks for your PR! By analyzing the annotation information on this pull request, we identified @tbekolay to be a potential reviewer

@tbekolay
Copy link
Member

tbekolay commented Jul 13, 2016

I remember spending a long time picking that constant... the issue that I found was that any value that worked well for a certain number of neurons worked poorly for a different number of neurons. I sort of got around it by scaling by axis height, but that was a bit of a hack; in actuality, it should probably scale by some combination of axis height and the number of neurons... I believe the marker size is in units of points squared, so there's probably a square root in there too?

@celiasmith
Copy link
Contributor Author

I checked this value for numbers of neurons from 2-100 and it seemed ok... i think that's a good 'reasonable' range. The previous value didn't look ok at all for small numbers of neurons.

@drasmuss drasmuss self-assigned this Jul 28, 2016
@drasmuss
Copy link
Member

Updated constant looks slightly better to me, here's an image for comparison (updated constant is bottom row):
comparison
You'll still get overlap for different sized plots, but at least this looks ok for the default plot size, and I'm not super motivated to do some fancy scaling to get it to work in all cases.

I also added a small fixup commit to update the documentation in that rasterplot function, which was wildly out of date. I squashed it in already (assuming it would be uncontroversial), as I was already fiddling with the history to update @celiasmith commit message to conform to our guidelines 😉. And rebased to master while I was at it.

@drasmuss drasmuss removed their assignment Jul 28, 2016
@celiasmith
Copy link
Contributor Author

man, that @celiasmith guy didn't even follow the guildines! what a douche. he was probably overwhelmed with the complexity of his added code.

@tbekolay
Copy link
Member

New plots LGTM; I pushed a fixup to add a changelog entry and fix up the example formatting a bit (interpreter sessions have a different starting marker for prompts versus continuations of a previous prompt). If it looks good to you @drasmuss fix it up and merge 👍 🌈

@tbekolay tbekolay assigned tbekolay and unassigned tbekolay Jul 29, 2016
@drasmuss drasmuss self-assigned this Jul 29, 2016
Also update example code in documentation
@drasmuss drasmuss merged commit 5c11cca into master Jul 29, 2016
@drasmuss drasmuss deleted the changed_marker_size branch July 29, 2016 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants