UWB-Biocomputing / Graphitti

A project to facilitate construction of high-performance simulations of graph-structured systems.
https://uwb-biocomputing.github.io/Graphitti/
Apache License 2.0
7 stars 14 forks source link

Improve regression testing setup to accommodate both CPU and GPU #358

Open stiber opened 2 years ago

stiber commented 2 years ago

Right now, we really just have things set up to do the automated CPU testing using GitHub actions. We will shortly have a shell script to do manual regression testing, which includes GPU-based build and test. However, none of the support file locations distinguish CPU vs. GPU. I suggest adding CPU and GPU subdirectories under Testing/RegressionTesting/GoodOutput at least (and maybe Testing/RegressionTesting/TestOutput; think about it) and modifying the actions and the manual shell script to work with this. This will allow direct CPU-CPU and GPU-GPU regression testing, in advance of dealing with issue #319.

NicolasJPosey commented 1 year ago

Thoughts on work to close this out:

  1. Add 'GoodOutput/CPU' and 'GoodOutput/GPU' directories to 'Testing/RegressionTesting/'.
  2. Move current xml files into 'GoodOutput/CPU'.
  3. Change all instances of 'GoodOutput' to 'GoodOutput/Cpu' in .github/workflows/tests.yml.
  4. Add a variable to 'Testing/RunTests.sh' called PROCESSING_UNIT='CPU'. Set this equal to 'GPU' if the -g flag is passed into the script. Append this variable to the GOOD_OUT_DIR variables so that we get the paths from 1.
stiber commented 1 year ago

And track how long the individual GPU regression tests take to run on raiju. We may want to add a test-large type of config file to the suite of GPU tests.

Oh, and also consider that the test-tiny and test-small runs might fail on the GPU; I'm not sure what happens when the number of threads in a kernel is less than one block.

NicolasJPosey commented 9 months ago

Implementation is in place. Currently blocked as the regression test is failing for both my branch and root/development branch so unsure if the failure is due to a change in code or is due to the known random number generator difference with CPU results. Discussed during lab meeting and path forward to close out this issue is to generate results from the current release using the GPU and add those results to the GPU test results folder in my branch.

NicolasJPosey commented 6 months ago

After reviewing commits and thinking more on this, I think the below is a summary of where we stand on this issue:

Given this, I proposal the following path to close out this issue:

  1. Understand the process used to validate the correctness between the old CPU output and the new CPU output.
  2. If this process can be used exactly for GPU outputs, then execute this process using old GPU output and new GPU output.
  3. If this process can NOT be used exactly for GPU outputs, then understand what (if any) modifications need to be made in order to validate the correctness of the new GPU output.
stiber commented 6 months ago

To the extent that this is merely a difference in XML output format, @xiangli6 should be able to advise regarding the changes and how to compare across formats. If we can bridge the comparison across formats to validate the development branch against the master branch, then we can subsequently rely on regression testing within the development branch (and within the modified master branch at a subsequent release).

xiangli6 commented 6 months ago

To the extent that this is merely a difference in XML output format, @xiangli6 should be able to advise regarding the changes and how to compare across formats. If we can bridge the comparison across formats to validate the development branch against the master branch, then we can subsequently rely on regression testing within the development branch (and within the modified master branch at a subsequent release).

The ongoing development of the new Recorder is currently focused on handling unprocessed spike history raw data. In contrast to the old output, where the spike history data was compiled network-wide in 10ms bins for all neurons (only some necessary data), the new Recorder exclusively deals with all the unprocessed raw data. The old Recorder processed this compiled data in its compileHistories method with some computation, a step not required by the new Recorder. To validate the data of the new Recorder, I gather unprocessed raw data from the old Recorder and compare it with the formatted output of the new Recorder. Despite differences in format, I compare the corresponding numbers. The distinction in format lies in how the spike history data is structured. In the old output, it is one cohesive chunk encompassing data for all neurons, whereas the new Recorder generates separate chunks of unprocessed raw data for spike history, each dedicated to an individual neuron, resulting in multiple chunks of data. Please notice that, for the constant variable (x y location) that I will address in the next step, both the format and data remain consistent between the old and new output.

NicolasJPosey commented 6 months ago

@xiangli6 I reviewed the CPU outputs and it's still not clear to me how one would go from the old format to the new one. I notice that the starterNeurons values in the old output correspond to the Neuron_# names in the new output. When I look at a neuron like Neuron_7, the first numerical value I see is 5900. I don't see this value in the old output which leads me to believe that it is a computed value. I am not sure how involved a single neuron validation step is but would it be possible to document the process required to validate a smaller neuron? I assume one could then take this process and apply it to each of the other neurons. In the development branch CPU output, it looks like Neuron_37 has the smallest number of elements in the test-small-out.xml file at 35 elements.

stiber commented 5 months ago

Talking to @xiangli6 , it's not just a format issue. The version in master doesn't output all of the spike data; it outputs "digested" data (because we didn't use XML format for fully detailed output). Xiang hacked the old code to dump the full raw data, to compare to the new Recorder output. Will need to re-implement that temporary hack to dump out raw data for the GPU simulations to compare to the new stuff and bridge the gap into the new Recorder (development) code base's regression testing.

xiangli6 commented 5 months ago

Currently, the updated version of XmlRecorder exclusively records raw data related to SpikeHistory, sourced from the variable 'vertexEvent' in the Vertice class. Specifically, it records all the data that stored in vertexEvent. The vertexEvent is a vector of EventBuffer and the index of this vector is the neuron number. Not all the neuron generate data(Events) so you see Neuron# names in the new output such as Neuron_7.

In contrast, the previous XmlRecorder processed data within 'vertexEvent_' and selected specific processed data for output in the old version(line 92 -95 here is the computation part for spikeHistory in the old version of XMLRecorder GitHub link).

To assess the changes, I eliminated the processing step and directed all raw data to a file named 'raw_data_old.txt' within the old XmlRecorder. Subsequently, I compared the numeric values in 'raw_data_old.txt' with the new output file. You can find the code (line 93-107) used for generating the raw data related to SpikeHistory in the old XmlRecorder version at this GitHub link." please notice that this code should be copied to old XmlRecorder to use.

NicolasJPosey commented 5 months ago

Thanks for this Xiang! I am looking over both links and have some questions

  1. It's obvious that lines 95-107 look virtually identical to the compileHistories method in master. However, line 103 is doing a push back call to a variablesHistory_ variable that is not defined in master. Line 104 looks like it's the raw data that we want to inspect. What/where should this be placed such that it will be output when the simulator is run?
  2. Since I want to inspect all the GPU files, would I be able to generate outputs for all the test files by simply running the GPU unit test once 1 is figured out? I know that the unit test will fail but I figured that is okay since I am more interested in inspecting the output files myself anyways.
NicolasJPosey commented 5 months ago

@xiangli6 can you take a look at my comment and reply back when you get a chance? Thank you!

xiangli6 commented 5 months ago

Thanks for this Xiang! I am looking over both links and have some questions

  1. It's obvious that lines 95-107 look virtually identical to the compileHistories method in master. However, line 103 is doing a push back call to a variablesHistory_ variable that is not defined in master. Line 104 looks like it's the raw data that we want to inspect. What/where should this be placed such that it will be output when the simulator is run?
  2. Since I want to inspect all the GPU files, would I be able to generate outputs for all the test files by simply running the GPU unit test once 1 is figured out? I know that the unit test will fail but I figured that is okay since I am more interested in inspecting the output files myself anyways.

Question 1. The spikesHistory in old class is the variableHistory in the new XmlRecorder class. ( line 95) Under the master branch, If you want to test the output with raw data regarding the spikeHistory variable. Here is the code you could use in the compileHistories() method: { AllSpikingNeurons &spNeurons = dynamic_cast<AllSpikingNeurons &>(vertices); Simulator &simulator = Simulator::getInstance(); int maxSpikes = static_cast(simulator.getEpochDuration() * simulator.getMaxFiringRate());

for (int iNeuron = 0; iNeuron < spNeurons.vertexEvents.size(); iNeuron++) { for (int eventIterator = 0; eventIterator < spNeurons.vertexEvents[iNeuron].getNumEventsInEpoch(); eventIterator++) {

     spikesHistory_[iNeuron] .push_back(static_cast<int>(static_cast<double>(spNeurons.vertexEvents_[rowIndex][eventIterator])));

  }

} spNeurons.clearSpikeCounts(); }

Yes, you are right. lines 95-107 is virtually identical to the compileHistories method in master. What I did just removing the computation part in the for loop.

xiangli6 commented 5 months ago

@xiangli6 can you take a look at my comment and reply back when you get a chance? Thank you!

Sorry for the late reply because I don't have answer for question 2. I'm not familiar with GPU unit testing and how it's different from CPU unit testing. If you want to compare the old and new output files, you'll need to make changes to the old Recorder class code because the data and format have changed. If you decide to check by yourself, I think it is ok.

I just want to mention that there are three types of data in the output file. First, there are constant variables that stay the same between the old and new files. Second, there are variables with computations that have changed, so you'll need to get raw data to compare them. And finally, there are variables that only contain necessary data, such as radiiHistory, which might only have data that meets a specific condition like "if (radii[iVertex] < minRadius)".

NicolasJPosey commented 4 months ago

@xiangli6 thanks for the feedback.

Regarding spikesHistory code snippet, it looks like this throws a compile error "expression must have class type but it has type "float"". It looks like spikesHistory is a VectorMatrix object and spikesHistory[iNeuron] returns a float instead of a vector so I changed "spikesHistory[iNeuron].pushback" to "spikesHistory[iNeuron] =" which appears to resolve the error.

RawChangesMaster

Assuming this was the right solution for resolving the compiler error, it's still unclear to me what numerical values and calculations need to be done to get from the old format to the new one. You mentioned that you "I compared the numeric values in 'raw_data_old.txt' with the new output file"; can you outline what values to compare for this step? I committed the raw GPU outputs I got with the above code in a RawGPUOut folder that lives in the same level as the GoodOutputs and TestOutputs folders in regression testing. These files were committed to my 358 branch.

xiangli6 commented 4 months ago

@NicolasJPosey

  1. If I remember correctly, I've updated the data structure in the XmlRecorder from a VectorMatrix named spikesHistory_ to a nested vector<vector<>> (maybe Vector<"EventBuffer"> which is still a 2-D). This change will require corresponding modifications in the code. To clarify, the original VectorMatrix was essentially a 1-D vector that sequentially stored data for all neurons across each epoch. For instance, with 2 neurons and 2 epochs, the data arrangement was like this: <Neuron1_Epoch1, Neuron2_Epoch1, Neuron1_Epoch2, Neuron2_Epoch2>. In contrast, the new structure organizes the data by neuron, then by epoch, resulting in an output format like: <{Neuron1_Epoch1, Neuron1_Epoch2}, {Neuron2_Epoch1, Neuron2_Epoch2}>. Let me know if this works.

  2. Regarding comparing numeric values between two files, I want to add that I also altered the output format of the old system to simplify the comparison process. My primary focus remained on comparing the numerical values.

stiber commented 4 months ago

@xiangli6 One question: it seems like right now the development branch is just recording a few specific neurons' EventBuffers. Does the Recorder now have the ability to record a vector of EventBuffers? What would that output look like?

xiangli6 commented 4 months ago

@xiangli6 One question: it seems like right now the development branch is just recording a few specific neurons' EventBuffers. Does the Recorder now have the ability to record a vector of EventBuffers? What would that output look like?

@stiber Hi Professor,

Currently, the XmlRecorder can receive a vector of EventBuffers by overloading the registerVariable method. However, within the Recorder, it separates each EventBuffer in the vector, adding each one to an element of the variable table, resulting in nearly identical outputs. The output consists of multiple data chunks, with each chunk representing the vertex event for each neuron. The only distinction lies in the variable name; given a vector of EventBuffers, the variable owner class can only pass through one variable name via the method. At present, the Recorder is only able to handle only 1-D vector variables.

We've discussed implementing a 2-D vector buffer here (https://github.com/UWB-Biocomputing/Graphitti/issues/595#issue-2104008865). I believe that introducing an additional variable table to accommodate such variables could be a possible solution. Since the Graphitti only support 1-D and 2-D and only two tables needed (the creation of a new variable table is solely associated with the dimensions of the variable).

stiber commented 4 months ago

Sounds good; we can add the 2D matter as an issue. Given that the HDF5 Recorder class implementation will require more information about the variables being stored, I suspect that developing that class will provide a solution for the XML Recorder.

We should definitely add "create unit tests" for the Recorder class as an issue. For regression testing, and overall operation, we should make sure that we register the vector of EventBuffers, rather than just the few we use for testing.

stiber commented 3 weeks ago

See https://github.com/UWB-Biocomputing/Graphitti/pull/639 for some of the reference files.