fix: synchronize R RNG state in hand-written C glue (#2211)#2674
Draft
schochastics wants to merge 1 commit into
Draft
fix: synchronize R RNG state in hand-written C glue (#2211)#2674schochastics wants to merge 1 commit into
schochastics wants to merge 1 commit into
Conversation
Hand-written wrappers in src/rinterface_extra.c called igraph functions that consume the RNG without bracketing them with GetRNGstate()/ PutRNGstate(), so R's seeded RNG state was never read into the default igraph RNG. This made layout_with_fr() and other stochastic functions return different results on each call even under withr::with_seed(). Adds an Rx_PutRNGstate_pv helper registered via IGRAPH_FINALLY so the RNG state is also restored along error paths that longjmp out of IGRAPH_R_CHECK. The bracket is applied to the FR, KK, graphopt, LGL, DrL, merge_dla and walktrap_community wrappers, plus the legacy RNG_BEGIN/RNG_END calls in Rx_igraph_ac_random_numeric are expanded to the explicit API. Replaces the fragile sum(l) snapshot test for layout_with_fr() with shape + finitude checks, hoists the local check_matrix helper, and adds reproducibility tests for every stochastic layout plus sample_gnp/gnm/pa/smallworld, random_walk, and cluster_walktrap/louvain/leiden. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
|
I noticed you are using the "finally" mechanism here. This should then also be done for the autogen code. Right now I do not see any of the necessary RNG sync in the autogen code, and I'm not sure why, as I thought this was already added. |
Contributor
|
This affects only 1.0, will be done as part of the incremental vendoring. |
Member
|
Yes, this affects only 1.0. |
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.
Closes #2211.
Problem
layout_with_fr()and other stochastic functions returned different results on each call even when seeded withset.seed()/withr::with_seed(). Root cause: since C core 1.0 it is the R interface's responsibility (not the C core's) to bracket each call that uses the RNG withGetRNGstate()/PutRNGstate(). Stimulus emits these in generated wrappers, but the hand-written glue in src/rinterface_extra.c never called them, so R's seeded state was never read into the default igraph RNG.The minimal reproducer from the issue:
Changes
C glue (src/rinterface_extra.c)
Rx_PutRNGstate_pvregistered viaIGRAPH_FINALLY, soPutRNGstate()runs along the error path thatIGRAPH_R_CHECKlongjmps through.igraph_*calls in:Rx_igraph_layout_fruchterman_reingold(+_3d),Rx_igraph_layout_kamada_kawai(+_3d),Rx_igraph_layout_graphopt,Rx_igraph_layout_lgl,Rx_igraph_layout_merge_dla,Rx_igraph_walktrap_community, and the DrL thin wrappersRx_igraph_layout_drl/_3d.RNG_BEGIN()/RNG_END()inRx_igraph_ac_random_numericexpanded to the explicitGetRNGstate()/PutRNGstate()API, per @szhorvat's note in the issue thread (item 4).Tests
expect_equal(sum(l), 4.57228, tolerance = 0.1)checks with shape + finitude assertions; hoisted the existing localcheck_matrixhelper to file scope; added astochastic layouts are reproducible with set.seed()block covering FR, KK, DH, GEM, graphopt, LGL, DrL in 2D/3D.sample_gnp/gnm/pa/smallworld,random_walk, andcluster_walktrap/louvain/leiden.Stimulus / generated code
The Stimulus-generated src/rinterface.c currently contains no
GetRNGstate/PutRNGstatecalls at all (grep -c GetRNGstate src/rinterface.c→ 0). @szhorvat noted in #2211 (comment) that Stimulus 0.23 should emit these. This PR does not regeneraterinterface.c— it only addresses items 3–6 from the issue (hand-written glue + tests).The full test suite passes on this branch, which suggests the RNG-using functions whose only wrapper lives in the generated file currently work in spite of the missing calls (likely because of where the default igraph RNG defers to R, see src/rrandom.c). Still worth a follow-up to either (a) regenerate
rinterface.cagainst Stimulus 0.23 to confirm what it produces today, or (b) check whetherno_rngflags infunctions.yamlare over-suppressing the calls. Action: a separate issue / PR for items 1–2 of #2211 should be opened.Test plan
R CMD INSTALL --preclean .clean buildlayout_with_fr()#2211 reproducer now returns identical layouts under repeatedwith_seed(42, …)devtools::test()—[ FAIL 0 | WARN 0 | SKIP 6 | PASS 8028 ](the 6 skips are pre-existing optional-dependency skips)🤖 Generated with Claude Code