test for `layout_with_fr()`

[maelle]

In #2058

The test fails, is the following surprising (the result changing every time):

library(igraph)
#> 
#> Attaching package: 'igraph'
#> The following objects are masked from 'package:stats':
#> 
#>     decompose, spectrum
#> The following object is masked from 'package:base':
#> 
#>     union

withr::with_seed(42, {
  g <- make_star(30)
  l <- layout_with_fr(g, niter = 500, dim = 3, start.temp = 20)
})
sum(l)
#> [1] -139.0961

withr::with_seed(42, {
  g <- make_star(30)
  l <- layout_with_fr(g, niter = 500, dim = 3, start.temp = 20)
})
sum(l)
#> [1] -275.7935

withr::with_seed(42, {
  g <- make_star(30)
  l <- layout_with_fr(g, niter = 500, dim = 3, start.temp = 20)
})
sum(l)
#> [1] -26.29641

^{Created on 2025-10-14 with reprex v2.1.1}

The test has an expectation on sum(l) which can't be true since the result changes. I searched for layout in the changelog.

@szhorvat

[szhorvat]

The test has an expectation on sum(l)

That's a somewhat silly way to test this function, as the test does not verify correctness in any way. It just detects changes in the output. I believe most layout functions have similar smoke tests at the moment, which were added due to lack of time.

Making good tests for these functions is difficult. Things that might be worth testing: (1) Is the output free of Inf/NaN? (2) Is the dimensionality of the output as expected, i.e. $n\times d$ points where $n$ is the vertex count, $d=2$ or 3 for 2D or 3D.

since the result changes

That is concerning and would be good to investigate. The function itself wasn't changed. I wonder if the RNG integration is broken.

1. Make sure that the latest C core commits is used (1.0.0 the earliest, but ideally the latest commit on main).
2. Make sure that the latest Stimulus is used (tag 0.23), and that it is properly generating GetRNGState() and PutRNGState() calls. Calling these is now the responsibility of the R interface, and is not done by the C core. Note that functions.yaml now has a no_rng flag for some functions. These have been verified not to use the RNG in any way, and generating GetRNGState() and PutRNGState() is skipped for performance reasons.
3. Review hand-written glue code, and add GetRNGState() and PutRNGState() to the beginning/end, in a manner similar to what Stimulus generates. Skip this ONLY when you are absolutely certain that none of the called C/igraph functions employ the RNG.
4. My hunch is that the problem is the lack of GetRNGState() and PutRNGState(). But if the above steps don't fix the issue, can you verify whether other functions that use the RNG also don't respond to setting the seed?

tl;dr This is almost certainly because manually written glue code did not have GetRNGState() and PutRNGState() inserted. I think we forgot to notify all of you, but I think we did notify @krlmlr. I suggest that 1, 2, and 3 above should be verified/fixed before proceeding with the update to the C core 1.0. It's good that this issue was caught.

[szhorvat]

This is where @ntamas updated Stimulus to produce the appropriate calls: igraph/stimulus@3ca4430

I just checked point 2., and the appropriate calls are being generated by Stimulus, here's an example:

rigraph/src/rinterface.c

Lines 738 to 740 in b458501

	GetRNGstate();
	IGRAPH_R_CHECK(igraph_extended_chordal_ring(&c_graph, c_nodes, &c_W, c_directed));
	PutRNGstate();

The next priority should be point 3. Add these calls to manually written glue code as well.

Additionally, these lines should be removed:

rigraph/src/rinterface_extra.c

Lines 47 to 48 in b458501

	#define RNG_BEGIN() GetRNGstate()
	#define RNG_END() PutRNGstate()

The RNG_BEGIN and RNG_END macros should be manually expanded to GetRNGstate and PutRNGstate (this is just a find-and-replace).

[maelle]

@krlmlr @schochastics could we investigate this together at the next hackathon?