uwb-biocomputing / graphitti Goto Github PK

Right now, it indicates it updates weights. This should be clarified to indicated that it takes a weight matrix already computed and is merely pushing those weights into the synapse data structures that are sitting on the device(s)

See issue UWB-Biocomputing/BrainGrid#232 to see the bug aspect of this. Currently, the Makefile is set to compile code using the C++98 standard. There is a problem with initialization of static const member variables, however. Also, generally, there can be some code cleanup and modernization (that might also have a small positive impact on performance). This is a worthwhile project.

What is affected by this?

Simulator

How do we replicate the issue/how would it work?

Expected behavior (i.e. solution or outline of what it would look like)

Other Comments

There should be a .h file for MersenneTwister_kernel.cu that declares its functions that can be called by external code.

What kind of issue is this?

• Bug report
• Feature request
• Question not answered in documentation
• Cleanup needed

What is affected by this?

Affects simulator. Currently, there is a CXX variable for g++. We should add a CXX_cuda variable so we can set the CUDA compiler. Then, the rules at the bottom of the Makefile should use CXX_cuda, instead of having "nvcc" hard-coded.

How do we replicate the issue/how would it work?

Expected behavior (i.e. solution or outline of what it would look like)

Other Comments

When the simulator is run, if it can't create the output file ("stateOutputFileName", from config file) at the end of thew simulation, it just terminates without any error message, leaving the reason why there's no output file a mystery. It at least should produce an error message. Even better would be for it to check at startup that it can create the file, terminating with a useful error message right away (rather than after several day's of chugging along).

What is affected by this?

Simulator

The name is fine, but maybe something like "saveResults" or a name that communicates that it's producing simulation result output files for other tools (for analysis, etc).

Right now, there is a #define in MersenneTwister.h, which should be removed, because it's really almost impossible to figure out what's going on with this code (and I'm sure that Doxygen can't figure this out).

ClusterInfo contains per-cluster data, rather than data global to all clusters. So, there's no need for this extra class; its contents can become part of Cluster.

What is affected by this?

simulator

How do we replicate the issue/how would it work?

Expected behavior (i.e. solution or outline of what it would look like)

Other Comments

Pulled this issue from an e-mail Don sent.

SparseMatrix::operator() creates a zero value element if the element doesn't exist. This appears to violate the spirit of what a sparse matrix should be. On the other hand, it isn't immediately clear what else to do if you want to use this as an Lvalue. Perhaps there is no better option. But, a const accessor version is an obvious approach to partially ameliorate this.

Then again, I don't remember what was going through my mind at the time and we don't want to assume I was completely confused at the time (no comments from the peanut gallery, please). Needs to be investigated thoroughly; maybe I will need to do this.

There are lines of code that access member variables like this->varname, because a local var/parameter has the same name. Would be better to change the local var/param name to avoid this.

Run the same description file () on cssgpu01 and cssgpu02p. The results were not identical.

With the idea that BG and config files get copied elsewhere, so runs don't "pollute" the git development directory.

Right now, we specify the main parameter file name on the command line. The neuron lists have relative path names inside that file. Those paths are relative to the working directory where BG is run, not where the parameter file is. We should come up with a way to make this more obvious, or we should do something like add a command line argument that specifies the parameter file directory and have the NList file paths be relative to that (and eliminate the need to provide the path in the command line argument for the main parameter file).

Perhaps leverage an open source C++ debug framework?

For example, default_Tau != default_Cm * default_Rm

Right now, we attach attributes such as "excitatory" or "inhibitory", etc. to the synapse, which is the "correct" way to do things. However, this doesn't work for the Izhikevich model, because there are some neuron parameters that vary by type. Right now, we encode this information in the Layout. We should think through how to deal with this in the long run.

See segundo86, figure 22 for starting point for many different synapse (actually, PSP) types. Need to do a little research about whether neurons can actually have different synapse types.

This should do the following:

1. Get information about simulation configuration (number of devices, which devices, which models, etc.)
2. Read device information for all requested devices
3. Allocate neurons to devices (incoming synapses live on the same device as their neuron)
4. Calculate threads per block, number of blocks per device.

How do we specify the specific devices to use in a way that makes sense and will work on multiple machines (not just raiju or cerberus)?

Ideally, a method/kernel like advanceNeuron(s) or advanceSynapse(s) should take in a function that returns the derivatives of the model state variable(s), and a function that implements an integrator, and then use those two to update the state variable(s). Right now, we've intermixed all of that into the model state variable update. With the (admittedly, not minor) change, the advance method/kernel could basically be inherited from the top of the class hierarchy.

This will require some thought, because we want it to be efficient on both the CPU and the GPU.

See http://nwb.org/resources/. NWB is built on top of HDF5, so this should not be an enormous problem.

This is the format used by the Allen Insitute, among others. It's being put forward as a standard for neuroscience data exchange and archiving.

For example, the GPU version doesn't have a AllSynapsesDevice class. This might be fine, but such inconsistencies are something to think about.

What kind of issue is this?

• Bug report
• Feature request
• Question not answered in documentation
• Cleanup needed

So, a few things about this class to look into:

1. Is the class name the best name to use for a factory class?
2. This is actually a set of four factory classes; will that be a problem as we refactor parameter reading?
3. This also implements a singleton pattern, using the static get() method. Is this the "classic" pattern implementation approach?
4. The code that registers the available neuron, synapse, connection, and layout classes is in the FClassOfCategory constructor. It would probably be better if this were in the classes that might be instantiated, to establish a good pattern for additional ones, to avoid the need for others to modify the factory, and to be consistent with our plans for registering parameters to a parameter reading class.
5. The createX() methods all read the name of the class to create and then call a createXWithName() method to do the instantiation. The class name reading needs to be moved into the "phase 1" parameter reading method.
6. The readParameters() method appears to be reading all of the instantiated classes' parameters, via the VisitEnter() method. This is the top-level core of the "phase 2" (model component instance) parameter reading code.

What is affected by this?

How do we replicate the issue/how would it work?

Expected behavior (i.e. solution or outline of what it would look like)

Other Comments

What kind of issue is this?

• Bug report
• Feature request
• Question not answered in documentation
• Cleanup needed

Right now, we have a high degree of nesting of objects in other objects, which means that operations done at a high level, for example from main(), must be passed down a long chain of method calls to reach the object that can actually perform the operation. We need to fix this.

What is affected by this?

How do we replicate the issue/how would it work?

Expected behavior (i.e. solution or outline of what it would look like)

Other Comments

Right now, the serialization file isn't opened until after the simulation runs. The same may be true for the results file. We should test that it's possible to open these files for writing before the simulation runs, so that the user doesn't have to wait until after the simulation to realize that they are in the wrong working directory.

What is affected by this?

How do we replicate the issue/how would it work?

Expected behavior (i.e. solution or outline of what it would look like)

Other Comments

The first level below the class in the parameter file is "LayoutFiles". This was copied from FixedLayout. However, the parameters below that are fractions, used to generate random layouts, not file names. The "LayoutFiles" tag should be changed to read "LayoutStatistics" or the like.

This is a precursor to investigating their licenses, whether those are problematic for us and our licenses, and what we need to do to be in compliance with their licenses. Please generate one additional issue for each package that we're using, so someone else can follow up with details.

Segregate files that would be modified from those that don't?
Precompile parts of the system?

What kind of issue is this?

• Bug report
• Feature request
• Question not answered in documentation
• Cleanup needed

In both code and parameter files.

###Code

• SimulationInfo: maxSteps -> numEpochs
• SimulationInfo: currentStep -> currentEpoch
• Simulator.cpp::simulate(): currentStep -> currentEpoch
• SimulationInfo: resultOutputFileName -> resultFileName
• SimulationInfo: resultInputFileName -> parameterFileName
• SimulationInfo: stimulusInputFileName -> stimulusFileName

###Parameter file

• numSims -> numEpochs
• Tsim -> epochDuration

What is affected by this?

How do we replicate the issue/how would it work?

Expected behavior (i.e. solution or outline of what it would look like)

Other Comments

Can we automate determining number of threads per block, etc. based on GPU capabilities? We should certainly, at least, query the GPU capabilities and gracefully exit, with informative feedback, if those capabilities don't match simulation requirements.

Method names are ad hoc. Let's make their names clearer, indicate if they apply to single synapses (class name implies all synapses, so no need to explicitly write that).

What is affected by this?

How do we replicate the issue/how would it work?

Expected behavior (i.e. solution or outline of what it would look like)

Other Comments

For example, we should be using a virtual architecture switch for the -arch option. See Options for Steering GPU Code Generation in the CUDA documentation.

What kind of issue is this?

• Bug report
• Feature request
• Question not answered in documentation
• Cleanup needed

Everything is now inherently multi-threaded (even if the number of threads is 1). We really mean, in this case, simulation thread(s) that run on the CPU.

What is affected by this?

How do we replicate the issue/how would it work?

Expected behavior (i.e. solution or outline of what it would look like)

Other Comments

It appears that many of the classes that read parameters have a checkNumParameters() function. Do all of them? How is this used as a sanity check? Assuming it's actually used, we could deal with some of the issues we discussed in our 5/23 lab meeting by at least implementing this function correctly for each class; this should be a quick thing.

Right now, there are a bunch of defaults for various parameters of LIF neurons and DS synapses. After checking that it won't break anything, these should be moved.

Maybe "braingrid" or "bg". Perhaps need a different name for GPU version?

See https://senselab.med.yale.edu/ModelDB/showmodel.asp?model=83319

What kind of issue is this?

• Bug report
• Feature request
• Question not answered in documentation
• Cleanup needed

Right now, simulator::finish() copies information back from the GPU to the CPU, frees memory on the GPU, and also triggers deletion of some CPU-side objects. We should separate the last of those three into a separate method, so we have one method that copies everything back from the GPU and frees the GPU storage (or maybe that should even be two methods) and a method that deletes CPU-side objects. In fact, maybe we can move all of the deletion of CPU-side stuff to destructors, which would accomplish that.

What is affected by this?

How do we replicate the issue/how would it work?

Expected behavior (i.e. solution or outline of what it would look like)

Other Comments

Use commit 4b0738cf045ca0c14cab877fba4d222987725842, AllDSSynapses_d.cu to try the following:

1. Compile without the assert() to PTX code.
2. Compile with assert()

How does the PTX code change? Is the code that calculates the psr value different?

There are two (sets of) RNG: rng in Globals.cpp, which is used to randomize parameters during setup, and is seeded with a hard-coded constant, and rgNormrnd/GPU Mersenne twister RNG(s) (created in SingleThreadedSpikingModel.cpp or GPUSpikingModel.cu), used to generate noise during simulation. The latter is seeded from the parameter file; the former is not.

@fumik added comments on AllNeurons.h and AllSynapses.h. There are four types of structure members, LOCAL CONSTANT, LOCAL VARIABLE, GLOBAL CONSTANT, and GLOBAL VARIABLE. All GLOBAL members can be scalars, not arrays. Also some structure members are neuron or synapse type dependent, so they can be optimized.

Need to also look at the units we use in the code, the difference between that and that Matlab reference code, and which makes sense for us to use.

Clean up the following:
//initialize Mersenne Twister
//assuming neuron_count >= 100 and is a multiple of 100. Note rng_mt_rng_count must be <= MT_RNG_COUNT
int rng_blocks = 25; //# of blocks the kernel will use
int rng_nPerRng = 4; //# of iterations per thread (thread granularity, # of rands generated per thread)
int rng_mt_rng_count = sim_info->totalNeurons/rng_nPerRng; //# of threads to generate for neuron_count rand #s
int rng_threads = rng_mt_rng_count/rng_blocks; //# threads per block needed

It seems some are totaled over the entire simulation; some are just the last kernel/function call. Make this consistent and have it produce an average per call.

Right now, the factory class's constructor has a bunch of calls to register the various types of neurons, etc. I think that it would be simpler conceptually if the registerX() calls were made from the neuron, etc. classes. That way, there would be a very clear implementation pattern for a new class, and no need to modify other code (like the factory class). Perhaps this would require a static method for each of those classes, plus a mechanism to trigger all of those static methods at program load time? I believe I have some musty old code that demonstrates this, in SourceVersions.

SourceVersions.zip

1. On the CPU, during simulation setup, for initializing parameters.
2. On the CPU, for CPU-based simulations, for noise.
3. On the GPU, for GPU-based simulations, for noise.

For example, every neuron has its own C3, which is just deltaT, right? Probably should audit the neuron and synapse data structures.