Skip to content

fix: synchronize R RNG state in hand-written C glue (#2211)#2674

Open
schochastics wants to merge 1 commit into
mainfrom
fix-rng-state-issue-2211
Open

fix: synchronize R RNG state in hand-written C glue (#2211)#2674
schochastics wants to merge 1 commit into
mainfrom
fix-rng-state-issue-2211

Conversation

@schochastics
Copy link
Copy Markdown
Contributor

Closes #2211.

Problem

layout_with_fr() and other stochastic functions returned different results on each call even when seeded with set.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 with GetRNGstate() / 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:

withr::with_seed(42, l1 <- layout_with_fr(make_star(30), niter = 500, dim = 3, start.temp = 20))
withr::with_seed(42, l2 <- layout_with_fr(make_star(30), niter = 500, dim = 3, start.temp = 20))
identical(l1, l2)  # FALSE before, TRUE after

Changes

C glue (src/rinterface_extra.c)

  • New static helper Rx_PutRNGstate_pv registered via IGRAPH_FINALLY, so PutRNGstate() runs along the error path that IGRAPH_R_CHECK longjmps through.
  • Bracket added around 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 wrappers Rx_igraph_layout_drl / _3d.
  • Legacy RNG_BEGIN() / RNG_END() in Rx_igraph_ac_random_numeric expanded to the explicit GetRNGstate() / PutRNGstate() API, per @szhorvat's note in the issue thread (item 4).

Tests

  • tests/testthat/test-layout.R: replaced the fragile expect_equal(sum(l), 4.57228, tolerance = 0.1) checks with shape + finitude assertions; hoisted the existing local check_matrix helper to file scope; added a stochastic layouts are reproducible with set.seed() block covering FR, KK, DH, GEM, graphopt, LGL, DrL in 2D/3D.
  • tests/testthat/test-rng-seeding.R: new file with seeded reproducibility tests for sample_gnp / gnm / pa / smallworld, random_walk, and cluster_walktrap / louvain / leiden.

Stimulus / generated code

The Stimulus-generated src/rinterface.c currently contains no GetRNGstate / PutRNGstate calls 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 regenerate rinterface.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.c against Stimulus 0.23 to confirm what it produces today, or (b) check whether no_rng flags in functions.yaml are 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 build
  • Issue test for layout_with_fr() #2211 reproducer now returns identical layouts under repeated with_seed(42, …)
  • devtools::test()[ FAIL 0 | WARN 0 | SKIP 6 | PASS 8028 ] (the 6 skips are pre-existing optional-dependency skips)
  • CI checks pass

🤖 Generated with Claude Code

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>
@szhorvat
Copy link
Copy Markdown
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test for layout_with_fr()

2 participants