refactor: adapt to cut.prob's new handling of NULL in the C core (sim…

[maelle]

…pler default for the R interface)

Fix #1570

Work needed in the tests.

[aviator-app]

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue label.

---

See the real-time status of this PR on the Aviator webapp.

Use the Aviator Chrome Extension to see the status of your PR within GitHub.

[maelle]

mmh this does not work at all currently.

[maelle]

@szhorvat actually, I think things are fine. What do you think of the tests

rigraph/tests/testthat/test-motifs.R

Line 1 in 30518c2

test_that("motif finding works", {

?

They're failing for small differences. Furthermore they do not make any sense to me, why are we testing for the value of the divisions?

── Failure ('test-motifs.R:11:3'): motif finding works ─────────────────────────
c(mno0/mno, mno1/mno, mno2/mno) (`actual`) not equal to c(0.654821903845065, 0.666289144345659, 0.668393831285275) (`expected`).

  `actual`: 0.67454 0.66614 0.66597
`expected`: 0.65482 0.66629 0.66839

[szhorvat]

I'm really tired today ... could you please help me by showing me a specific before/after example that changes output? Passing c(0,0,0) vs NULL should NOT change anything.

But as I'm writing this, I think I'm starting to remember what's going on:

I think passing NULL instead of c(0,0,0) vs will cause some (unnecessary) RNG calls to be omitted. This means that later calls that use different values than c(0,0,0), and therefore return stochastic results, should indeed be affected. And yes, this is not a bug, don't worry.

It'll be cleanest for each test to use its own random seed.

[szhorvat]

Yes, this is certainly what's going on. If you pass NULL or c(0,0,0,...), the result will be the exactly same. But the RNG state will be mutated differently, which means that any subsequent uses of the RNG are affected.

The results are approximately the same and everything is fine.

Adding a tolerance won't work very well here because the noise in the results is still quite high and will continue to be high unless we use large enough graphs and small enough cut probabilities that the computation time becomes too long for a test.

[szhorvat]

Furthermore they do not make any sense to me, why are we testing for the value of the divisions?

The interface is not very nice, unfortunately, but improvements are for a later version and for the C core.

If we give cut probabilities $p_1, p_2, \dots$, then only a fraction of the motifs will be sampled. This fraction is $\prod_i (1-p_i)$. This is the value you should see in the ratio of the counts obtained with a non-zero cut probability and the full counts (with no cuts). Since some motifs are rare, some entries in the result vector will fluctuate wildly.

So, if you give c(1/3, 0, 0), then the ratios should all be about $1-1/3 \approx 0.66$.

[szhorvat]

Instead of putting the call in a conditional, update the value of cut.prob in a conditional, and keep a single call to the C function.

[szhorvat]

When you resolve conflicts, be sure that you don't accidentally re-add as.numeric to sample.