Skip to content

Conversation

@hqkhan
Copy link

@hqkhan hqkhan commented Feb 25, 2018

I brought over plotting.py from spiketorch as a baseline. For starters, I modified plot_spikes to do a few more things that are explained inside the code. Also, in "simple test network.ipynb" I am showing the new changes (being able to plot for a certain amount of time). I'm creating a pull request to get some feedback from all of you. I thought being able to narrow down specific times was going to be a useful feature. I can do the same for plot_voltages.

I think it is not necessary to have user give their own figure to plot on. I feel like I should remove ims from the function. Thoughts?

Here are a few questions: What other plots are we interested in? Plots for weights and others are essentially implemented. Even voltages one is not hard. I can do the same thing on them to be able to modify timings of plots.

What kind of stats are we interested in? Stats such as mean voltages of all neurons plotted over time?

@hqkhan hqkhan changed the title Hassaan Plotting functions (feedback needed, so I can modify the others) Feb 25, 2018
@djsaunde
Copy link
Collaborator

djsaunde commented Feb 26, 2018

This is a good first step.

The first time the function is called, figures are created and returned to the user. This can be seen from the if not ims: statements (in retrospect, these should be change to if ims is None:. So, I don't think this is requiring too much. If you can figure out a simple way to create the figures from within the package and maintain them "behind the scenes" (i.e., without passing them back to the user), that would be ideal. If the figures were recreated each time the function were called, this too would be problematic, as it would add lots of computational overhead, as opposed to simply changing the data in the figures and re-drawing.

As for future plots and statistics, this should be discussed at our next subgroup meeting. I would like to see a function for plotting an arbitrary recorded variable over time, e.g., from a Monitor object. This would remove the need to have separate functions plot_spikes() and plot_weights(), etc., and might be called plot_recording() or something like that.

This might not be possible: the tricky part is the separate configuration for each type of recorded value; e.g., spikes should be plotted using plt.matshow(), while voltages should be plotted with plt.plot(). But for values that aren't common, I think that having a generic plotting function would still be useful.

@djsaunde
Copy link
Collaborator

@hqkhan If you can resolve the merge conflicts on this branch, I will approve this pull request.

@hqkhan
Copy link
Author

hqkhan commented Feb 26, 2018

@djsaunde I completely agree. I wanted to do a generic plotting function but there would need to be a structure in how the variables are stored inside monitor. For instance, we could add more attributes to the monitor class, sort of like keywords that the plotting function looks for and uses appropriate plt.matshow() or plt.plot() appropriately and even titles, axis labels to go along with it. But I'm not sure if we want that to be a part of monitor itself. This is almost turning the monitor class into a plotting class sort to speak. Currently using plt.imshow() instead of plt.matshow(). I can switch if needed but the axis need to be played around with using matshow which is why I went with imshow.

I did this plotting function just to showcase an idea of looking at a specific slice of time (it was also more so of me familiarizing with coding with this package).

Also, the "ims" part now makes a lot more sense. The number of plots created could be equal to the number of monitor variables. Any extra plots that the user wants, would need to be created on the fly I image. Yup, it's not difficult at all, I was simply inquiring the reasoning behind it.

@djsaunde
Copy link
Collaborator

Makes sense. It might be possible to have keyword arguments instead in a generic plot_recording() such as type (matshow, imshow, plot, scatter, etc.), figsize, xlabel, ylabel, title, etc. Of course, this would take some time and careful thinking to implement.

@hqkhan
Copy link
Author

hqkhan commented Feb 26, 2018

True, that would make more sense since they are plotting arguments. Yes, definitely but the final result I imagine would be very good given things work out. I will resolve the conflict and then work on something bigger.

@hqkhan
Copy link
Author

hqkhan commented Feb 26, 2018

I'm not exactly sure how to merge my changes. The conflict is within the notebook. I've moved my additions at the bottom of the document so that there are no overlapping potentially.

@djsaunde
Copy link
Collaborator

Hm, the conflicts still exist. You'll have to (locally) do a git merge master and then git add -A, then push again.

@hqkhan hqkhan merged commit d982921 into master Feb 26, 2018
@hqkhan hqkhan deleted the hassaan branch February 26, 2018 18:31
@djsaunde djsaunde restored the hassaan branch February 26, 2018 18:37
@djsaunde
Copy link
Collaborator

djsaunde commented Feb 26, 2018

It looks like you merged into master, and deleted your branch... I restored your branch (you're going to need it later), but I guess it's fine to merge your commits. In general, I think Hananel should approve your changes before you merge into master.

@hqkhan
Copy link
Author

hqkhan commented Feb 26, 2018

So what I did was, I merged, deleted my branch, updated my local master branch and created another branch off of it. Sorry about that, I can create a pull request for reverting the changes.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants