search

fzenke / auryn

Auryn: A fast simulator for spiking neural networks with synaptic plasticity
https://fzenke.net/auryn/
GNU General Public License v3.0
98 stars 42 forks source link

Ala mpi patch #31

Closed anderslan closed 10 months ago

anderslan commented 6 years ago

Patch for the spike communication using mpi. Major change in SyncBuffer but some in System.

fzenke commented 6 years ago

Hi Anders, I manually merged your connection object with some minor changes to the way how you handle traces. This is currently all in a separate development branch available here: https://github.com/fzenke/auryn/tree/dev-bcpnn What do you think? Best, F

anderslan commented 6 years ago

?Hi Friedemann,

OK, but I have now discovered (after my last post in the forum about everything working) that it crashes when running with more cores. So please wait a bit until you add it to the distribution. My fault I did not test early on. It is the evolve() function that crashes and I think one need to test on ranks there. Looking at it now - if you can easily spot the error in this method please tell me. Will be solved!

Best! /Anders La

Från: Friedemann Zenke notifications@github.com Skickat: den 25 augusti 2018 12:25 Till: fzenke/auryn Kopia: Anders Lansner; Author Ämne: Re: [fzenke/auryn] Ala mpi patch (#31)

Hi Anders, I manually merged your connection object with some minor changes to the way how you handle traces. This is currently all in a separate development branch available here: https://github.com/fzenke/auryn/tree/dev-bcpnn What do you think? Best, F

- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/fzenke/auryn/pull/31#issuecomment-415959402, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AXJjZGB1M0YNfslo77hSnx0Czwa4X7I0ks5uUSX9gaJpZM4V869e.

anderslan commented 6 years ago

Dear Friedemann,

I use github often to backup intermediate state to not lose changes. So when this code is properly tested I will notify you. And then commits after that date should be ignored. I am not used to this kind of code collaboration and not very good at git so you have to be patient with me - or maybe give some hints how I should go about...

Best! /Anders La

Från: Friedemann Zenke notifications@github.com Skickat: den 25 augusti 2018 12:25 Till: fzenke/auryn Kopia: Anders Lansner; Author Ämne: Re: [fzenke/auryn] Ala mpi patch (#31)

Hi Anders, I manually merged your connection object with some minor changes to the way how you handle traces. This is currently all in a separate development branch available here: https://github.com/fzenke/auryn/tree/dev-bcpnn What do you think? Best, F

- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/fzenke/auryn/pull/31#issuecomment-415959402, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AXJjZGB1M0YNfslo77hSnx0Czwa4X7I0ks5uUSX9gaJpZM4V869e.

fzenke commented 6 years ago

No worries. Your code will remain isolated in its separate branch until it is stabilized before it is merged in to the main development branch. Good luck bugfixing.

anderslan commented 6 years ago

Dear Friedemann,

I suppose this is not the right way to answer your code comment, but I looked around how to do it properly in git but failed miserably. Please give a hint or a link where I can learn about this. Meanwhile I will quickly reply to some of your comment, please see >>> below.

I am currently setting up some of our network models for testing.

I also plan to test increasing the time step in BcpnnConnection.evolve(), think this may speed up about 10x without sacrificing accuracy much.

Best/Anders La

Från: Friedemann Zenke notifications@github.com Skickat: den 1 september 2018 11:10 Till: fzenke/auryn Kopia: Anders Lansner; Author Ämne: Re: [fzenke/auryn] Ala mpi patch (#31)

@fzenke commented on this pull request.

I had a look at your changes and left a few questions open for discussion. Cheers, F

In src/auryn/Trace.hhttps://github.com/fzenke/auryn/pull/31#discussion_r212642380:

@@ -41,10 +41,12 @@ class Trace : public AurynStateVector protected: /! Decay time constant in [s]. / AurynFloat tau;

  • / Increment value in inc() /
  • AurynFloat kinc; // Added by ALa

Do we really need this kinc here? One can always multiply the trace value by kinc when using it. This often allows to combine it with additional multiplicative constants and thus reduce the number of operations. Probably it doesn't have a large effect on performance, but I would like to test this because it will affect all simulations ever written with Auryn. A possible implementation of your rule without introducing kinc is here: https://github.com/fzenke/auryn/tree/dev-bcpnn

In examples/sim_2comp_input.cpphttps://github.com/fzenke/auryn/pull/31#discussion_r212643406:

@@ -1,72 +0,0 @@ -/*

I can't merge this pull request as is because it deletes several example files. I will probably have to resort to a manual merge.

In examples/sim_brunel2k_pl.cpphttps://github.com/fzenke/auryn/pull/31#discussion_r214508547:

@@ -1,292 +0,0 @@ -/*

Won't merge. Do not see a reason why all examples should be deleted. I am assuming this was unintentional.

I did so because the turnaround time for compilation and linking was long with all those examples. I kept only those needed for my testing. Of course the others should not be removed... Probably a too quick and dirty way to do it!

In src/auryn/ComplexMatrix.hhttps://github.com/fzenke/auryn/pull/31#discussion_r214508576:

@@ -691,9 +691,10 @@ void ComplexMatrix::fill_zeros() template bool ComplexMatrix::exists(NeuronID i, NeuronID j, NeuronID z) {

  • if ( get_data_index(i,j) == data_index_error_value || z >= get_num_synaptic_states() )
  • if ( get_data_index(i,j) == data_index_error_value || z >= get_num_synaptic_states() ) {
  • // std::cerr << "ERROR: Element i = " << i << " j = " << j << " z = " << z << " non-existent!\n";

No real change here from what I can tell.

In src/auryn/SparseConnection.cpphttps://github.com/fzenke/auryn/pull/31#discussion_r214508624:

@@ -503,7 +508,7 @@ void SparseConnection::finalize() std::stringstream oss; oss << get_log_name() << "Finalized with fill level " << w->get_fill_level(); auryn::logger->msg(oss.str(),VERBOSE);

  • if (w->get_fill_level()<WARN_FILL_LEVEL)
  • if (false && w->get_fill_level()<WARN_FILL_LEVEL) // ALa changed

We can change the message to DEBUG, but should remove the "false" here.

Yes, I just got disturbed by all the warnings

In src/auryn/StateMonitor.cpphttps://github.com/fzenke/auryn/pull/31#discussion_r214508654:

{

    if ( !source->localrank(id) ) return; // do not register if neuron is not on the local rank

Why did you move output file generation to here? This was handled in Monitor::init nicely I thought. Did this create any problems for you? This way we need to replicate code (a call for open_output_file) in each descendant of Monitor.

Well, when running with many cores I got a lot of empty files, so this was done to prevent this. I think it is needed.

In src/auryn/StateMonitor.hhttps://github.com/fzenke/auryn/pull/31#discussion_r214508718:

@@ -130,6 +139,28 @@ class StateMonitor : public Monitor void execute(); };

+/! \brief Records from an arbitray pretrace vector of one unit from the source SpikingGroup to a file./ +class PreTraceMonitor : public StateMonitor

What do we need those for? StateMonitor can take an arbitrary Trace as argument and record from it. It seems like this Class isn't doing anything. If this is for readability I would suggest putting it as a typedef into the simulation code where it's being used.

Yes, for readability.

In src/auryn/SyncBuffer.cpphttps://github.com/fzenke/auryn/pull/31#discussion_r214508824:

@@ -71,6 +71,7 @@ void SyncBuffer::init() syncCount = 0;

    max_send_size = 4;

I have commited a draft version of a new SyncBuffer which is currently undergoing testing https://github.com/fzenke/auryn/tree/dev-syncbuffer - this version doesn't declare a second send_bufx array. Maybe you can check it out and tell me whether it works for you. If my suggested implementation works with your setup I would prefer it over the one in which an additional send_bufx is declared.

Agree, will test it

In src/auryn/SyncBuffer.hhttps://github.com/fzenke/auryn/pull/31#discussion_r214508848:

@@ -58,6 +58,7 @@ namespace auryn { { private: std::vector send_buf;

  • std::vector send_bufx; // ALa added

See also my comment above and my suggested implementation here: https://github.com/fzenke/auryn/tree/dev-syncbuffer

In src/auryn/auryn_definitions.hhttps://github.com/fzenke/auryn/pull/31#discussion_r214508917:

@@ -88,7 +88,7 @@

  • Furthermore groups that are distributed will not be cut up in chunks that
  • are not smaller than this. This is done to reduce overhead */ -#define DEFAULT_MINDISTRIBUTEDSIZE 16 +#define DEFAULT_MINDISTRIBUTEDSIZE 1 // ALa changed

And? Did this improve things?

-

No, Now changed back. You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/fzenke/auryn/pull/31#pullrequestreview-149321327, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AXJjZGxA8k1OD8Xuxxx3sEidy2BCwHEbks5uWk7-gaJpZM4V869e.

anderslan commented 6 years ago

?OK, forgot to mention that I tried to figure out a way of scaling the traces at computation of Wij and Bj, but I was not successful in changing that. So for now 'kinc' is needed there it seems.

/ALa

Från: Friedemann Zenke notifications@github.com Skickat: den 1 september 2018 12:57 Till: fzenke/auryn Kopia: Anders Lansner; Author Ämne: Re: [fzenke/auryn] Ala mpi patch (#31)

@fzenke commented on this pull request.

In src/auryn/Trace.hhttps://github.com/fzenke/auryn/pull/31#discussion_r214510683:

@@ -41,10 +41,12 @@ class Trace : public AurynStateVector protected: /! Decay time constant in [s]. / AurynFloat tau;

  • / Increment value in inc() /
  • AurynFloat kinc; // Added by ALa

Update: I tested it now. Speed-wise this change would not be a problem and we can discuss it. It may break the interface though for LinearTrace. Normally all traces can be swapped out with to event-based ones. Still have to think about it.

- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/fzenke/auryn/pull/31#discussion_r214510683, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AXJjZMjzPcsc210fiR3QItHcXBmEQ5Uwks5uWmgDgaJpZM4V869e.