Skip to content

Indices exchange in plot_spikes function#415

Merged
Hananel-Hazan merged 1 commit intoBindsNET:masterfrom
ruteee:master
Sep 23, 2020
Merged

Indices exchange in plot_spikes function#415
Hananel-Hazan merged 1 commit intoBindsNET:masterfrom
ruteee:master

Conversation

@ruteee
Copy link
Copy Markdown
Contributor

@ruteee ruteee commented Sep 23, 2020

Indices exchange in plot_spikes function, so that time varies from 0 to time param, and n_neurons vary from 0 to the number of neurons at each layer.

… time param, and n_neurons vary from 0 to the number of neurons at each layer
@Hananel-Hazan
Copy link
Copy Markdown
Collaborator

Grate Thank you

@davidAmiron
Copy link
Copy Markdown

When I run the example code at the end of this tutorial I get incorrect output; the number of neurons and time are switched, it seems to be as a result of this PR. When I reverse what was done here, I get the correct output. I see this is related to the reservoir mnist example.

Perhaps this is a problem in data formatting in the reservoir mnist example, or maybe some style changed and the example in that tutorial must be updated, I am not sure.

@Hananel-Hazan
Copy link
Copy Markdown
Collaborator

Hananel-Hazan commented Oct 6, 2020

Thanks @davidAmiron for pointing it out, you are right, the reservoir example had wrong plotting dimensions. Fixed in #417

Regarding the code in the tutorial, its seems running fine
image

Let me know if you have more issues

@ruteee
Copy link
Copy Markdown
Contributor Author

ruteee commented Oct 6, 2020

Hi, David.

I did the PR for this update. I tested the code example of the tutorial you mentioned and you are right the indices seem to be switched.

However if run reservoir example with indices switched back to their original positions (like the old version), the plot_spikes and the plot_voltages generate the incorrect output. That is with the neurons in the place of time, and vice-versa.

I agree there is probably something happening in data formatting in the reservoir example. I apologize for the mistake and suggest the reversion of PR.

@ruteee
Copy link
Copy Markdown
Contributor Author

ruteee commented Oct 6, 2020

@Hananel-Hazan should I do the PR for undoing the modification?

@davidAmiron
Copy link
Copy Markdown

I have a branch with the changes in it if you would like me to submit it

@Hananel-Hazan
Copy link
Copy Markdown
Collaborator

Hananel-Hazan commented Oct 6, 2020

@davidAmiron can you post a screenshot of the plot issue you having?
@ruteee Thank you for offering, I want to see the problem first, the plot I attached here look fine to me

@davidAmiron
Copy link
Copy Markdown

Running the example from the tutorial on master, I have the following:
image
For the plot_spike function, the number of neurons and number of timesteps are switched. If you run the example and print out spikes[key].shape right before line 101 of plotting.py and val.shape before line 107 of the same file, you can see that the order of number of neurons and number of timesteps is switched from what is being read in those lines, respectively.

@Hananel-Hazan
Copy link
Copy Markdown
Collaborator

I am sorry, I dont get the same plotting. Are you sure you are using the latest update? The screenshots that I getting from the tutorial look fine, they are posted here.

@davidAmiron
Copy link
Copy Markdown

Hmm. I am working directly off of master. If you are using a previous pip installation maybe #415 isn't on your machine, because it is only 12 days old?

@Hananel-Hazan
Copy link
Copy Markdown
Collaborator

Sorry, something went wrong with my GitHub merge that mask this update. @ruteee please revert the change. Thank you @davidAmiron for pointing it out

@ruteee
Copy link
Copy Markdown
Contributor Author

ruteee commented Oct 6, 2020

yes, for sure.

ruteee added a commit to ruteee/bindsnet that referenced this pull request Oct 6, 2020
Hananel-Hazan added a commit that referenced this pull request Oct 6, 2020
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.

3 participants