fix: stop using InfoNode.index as subnetwork scratch state#398
Draft
fix: stop using InfoNode.index as subnetwork scratch state#398
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stop using
InfoNode.indexas scratch storage while generating sub-Infomap networks.InfomapBase::generateSubNetwork(InfoNode&)previously overwrote each child node'sindexwith its temporary position in the new subnetwork, then reused that mutated state to clone internal edges. That worked as a local shortcut, but it also leaked a write to a field that carries real optimizer and tree state elsewhere in the codebase.This change replaces that side-channel with a local
InfoNode* -> positionmap and adds a regression test that proves subnetwork generation no longer clobbers parent node indices.Problem
The old implementation did this during subnetwork construction:
node.index = childIndexedge.source->index/edge.target->indexto find the cloned endpointsThat meant
generateSubNetwork(...)mutated caller-visibleInfoNode.indexstate as an implementation detail and never restored it.Why that is a bug:
InfoNode.indexis not scratch-only stateEven when the mutation does not immediately surface as a user-visible partition bug, it is still state corruption at the function boundary.
Fix
In src/core/InfomapBase.cpp, replace the temporary
node.indexwrite with a localstd::unordered_map<InfoNode*, unsigned int>:indexvalues untouchedTests
Add a regression in test/cpp/test_infomap_wrapper.cpp that:
indexvalues on the parent module's childrenAlso add the required
-fno-access-controlcompile option for the lifecycle test target in CMakeLists.txt so the regression can exercise the internal path directly.Risk
Medium.
This changes C++ core logic, but the surface is tightly bounded:
InfoNode.indexThe new mapping is local to subnetwork generation and does not change the cloned graph semantics.
Verification
Ran:
PATH="/opt/homebrew/bin:$PATH" make test-native JOBS=1Why This Should Merge Independently
This fix stands on its own.
It is not inherently tied to the larger CSR experimentation branch; that work only made the hidden coupling around
InfoNode.indexmore obvious. Removing the side-channel is independently valuable because it:generateSubNetwork(...)respect its caller's state