[issue-722] Fix default recorder variable setup#939
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves Graphitti’s default recorder variable setup (issue #722) so that key simulation outputs for STDP/static edges, growth simulations, and NG911 layouts are registered and populated with meaningful data before recorder output is saved.
Changes:
- Populate per-epoch active-edge recorder vectors in
ConnStatic::updateConnections()using active edges fromAllEdges. - Register growth (
radii,rates,outgrowth,deltaR) as dynamic recorder variables and NG911 layout coordinates as constant recorder variables viaLayout911::registerHistoryVariables(). - Update recorder documentation and the NG911 configuration example to reflect the current generic
XmlRecorderpath.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Simulator/Layouts/NG911/Layout911.h | Adds recorder mirror vectors and declares registerHistoryVariables() override for NG911 coordinate recording. |
| Simulator/Layouts/NG911/Layout911.cpp | Populates recorder mirrors during setup and registers x_Location/y_Location as constant outputs. |
| Simulator/Edges/AllEdges.h | Adds const accessors for edge source/dest indices, weights, and in-use flags to support recorder extraction. |
| Simulator/Connections/Neuro/ConnStatic.h | Declares updateConnections() override to populate per-epoch edge recorder outputs. |
| Simulator/Connections/Neuro/ConnStatic.cpp | Implements per-epoch active-edge list extraction into recorder vectors. |
| Simulator/Connections/Neuro/ConnGrowth.cpp | Registers growth-related VectorMatrix values as dynamic recorder variables. |
| docs/User/configuration.md | Updates NG911 example to use XmlRecorder instead of legacy Xml911Recorder. |
| docs/Notes/RecordersNotes.md | Updates recorder notes to describe active-edge output, growth variables, NG911 coordinate recording, and legacy Xml911Recorder status. |
|
|
||
| bool ConnStatic::updateConnections() | ||
| { | ||
| AllEdges &edges = getEdges(); |
|
I added a |
|
I looked into the regression failure starting with test-tiny, since that was the smallest case to check first. For test-tiny, the generated output is larger because it now includes the four ConnGrowth matrices this PR adds: radii, rates, outgrowth, and deltaR. The current GoodOutput file does not have those yet. The size changes from 468 bytes to 955 bytes, so the added growth output is +487 bytes for this tiny case. I also generated the NG911 test-small-911 output to make sure the growth matrices were not showing up there. They are not present in the NG911 output. That file changes from 99,043 bytes to 99,407 bytes, so +364 bytes, which appears to be from the expected coordinate output change. So from what I can tell, the CI failure is from the new growth recorder output not being in the current GoodOutput baselines yet. I wanted to check this first before updating any baselines, since the remaining question is whether these growth variables should be accepted as default regression output or handled differently. |
Summary
This PR fixes gaps in the default recorder setup for issue #722. The main problem was that some recorder variables were either not registered by default or were registered but not actually being filled with useful data before output was saved.
Changes include:
What changed
For STDP/static edge recording,
ConnStatic::updateConnections()now fills the recorder vectors for edge weight, source vertex, and destination vertex. It pushes one value per active edge into three parallel vectors:WCurrentEpoch_sourceVertexIndexCurrentEpoch_destVertexIndexCurrentEpoch_For the same index, those three values describe the same active edge for that epoch.
For growth simulations, this PR registers the existing growth values
radii,rates,outgrowth, anddeltaRas dynamic recorder variables.For NG911,
Layout911already hadxloc_andyloc_, but those areDeviceVector<BGFLOAT>values and cannot be registered directly with the recorder. This PR addsxlocRecorder_andylocRecorder_as recorder-compatible mirrors. These do not replace the original layout coordinates; they only make the coordinates available to the recorder.The
.assign(numVertices_, 0)calls size those recorder mirror vectors to one value per vertex before they are filled from the GraphML coordinates.Scope
This PR keeps the fix focused. It does not:
Documentation
Updated
docs/Notes/RecordersNotes.mdto make the issue #722 recorder behavior easier to follow, including active-edge output, growth dynamic values, NG911 coordinate recording, and Xml911Recorder’s legacy status.Updated the NG911 user config example to use
XmlRecorder.Verification
git diff --checkcmake --build buildcd build && ./tests— 133 tests passedRelated to #722