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

[maelle]

Simpler default for the R interface

Fix #1570
Fix #1574

How to handle 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]

@szhorvat for tests, is the idea simply to

• use a fixed random seed for each of the tests that's currently failing
• record the results as the new truth?

[szhorvat]

• use a fixed random seed for each of the tests that's currently failing

• record the results as the new truth?

This would make the tests more robust to changes of the sort we made. I think it's a good approach.

[maelle]

@szhorvat in that case, this is now ready for review. There were actually already fixed seeds.

[szhorvat]

I'm very tired, so expect some stupid comments from me, but I'll try to do a basic review.

[szhorvat]

Please help me understand the code below. Is it repeating the last entry to fill up the vector until it reaches size?

What if it is longer than size? That should be an error (and I think the C core will take care of that, but I'd need to check).

[szhorvat]

This just errors:

motifs(make_ring(10), cut.prob=0.1)
Error in motifs(make_ring(10), cut.prob = 0.1) : 
  At vendor/cigraph/src/misc/motifs.c:156 : Cut probability vector size (0) must agree with motif size (3). Invalid value

Why does it not trigger the length(cut.prob) != size code path?

I think I'm just too tired for this right now :(

[szhorvat]

This code path is triggered, but it produces an incorrect vector.

I know this is not your code, but can you fix it @maelle ?

My recommendation is:

1. Either go with what this code seems to intend, i.e. repeat the last element as many times as necessary to reach size.
2. Or keep things simple and not repeat anything except a scalar. A scalar should be treated as a vector of identical elements of length size.

No preference as to which from my size (maybe 1 is a tiny bit nicer, but also overly complicated?)

Whatever you choose, can you document it? Also, can you make sure it's implemented in all motif functions, not just this one?

[maelle]

So do you mean

• error if cut.prob is not of length either 1 or the same as size?
• if it is of length 1, repeat it size times?

[szhorvat]

That is option 2.

Let me give examples for option 1 and option 2, assuming size == 4.

Option 1:

Input: 0.3, interpreted as 0.3 0.3 0.3 0.3

Input: 0.3 0.4, interpreted as 0.3 0.4 0.4 0.4

Input: 0.1 0.2 0.3 0.4 0.5 (longer than size 4), should trigger an error, which can be achieved by passing to the C core as-is

So, repeat the last entry to fill to size.

Option 2:

Input: 0.3, interpreted as 0.3 0.3 0.3 0.3

Input: 0.3 0.4, passed to the C core as-is, which will trigger an error.

[maelle]

@krlmlr any opinion here?

[szhorvat]

I think sample.size should default to NULL in this deprecated function as well, to match sample_motifs.

Do what you think is best, just drawing attention to this.

[szhorvat]

• use a fixed random seed for each of the tests that's currently failing

I meant, the only way this issue could have been avoided is if a seed is set before every call to motif functions. We have the unusual situation here where an internal change did not affect the output, but it did affect how the function propagates the random state.

Actually setting the seed before every call is overkill, and I do NOT recommend it. It's good as it is IMO.

[szhorvat]

I looked at the changes in this PR and I think they're good. Thank you @maelle ! The issues I commented about are not due to your changes, but should still be fixed.