search

stocnet / manynet

Many Ways to Make, Manipulate, and Map Myriad Networks
https://stocnet.github.io/manynet/
Other
12 stars 0 forks source link

Invalid use of `igraph::sample_islands()` #86

Closed szhorvat closed 2 months ago

szhorvat commented 2 months ago

It seems that manynet's is using igraph::sample_islands() incorrectly. Specifically, the islands.size parameter must be a single integer (i.e. a scalar, not a vector).

https://github.com/stocnet/manynet/blob/ce9189fc92ee7d5ac212352e1bf3ce99540dce59/R/make_generate.R#L270

Somewhere along the way, more stringent parameter checks were added for this in igraph, which now causes the manynet test suite to fail with the upcoming version of igraph. Please see our revdepcheck results here.

Let me know if you have questions 😊

jhollway commented 2 months ago

So it isn't possible to create an even number of islands with an odd number of nodes?

szhorvat commented 2 months ago

That is possible. The problem is that manynet is passing a vector to islands.size. It should pass a scalar. c(3,4,5) is invalid, 3 is valid.

szhorvat commented 2 months ago

Perhaps the misunderstanding was that you assumed that different islands can be of different sizes? That is not the case. All of them will be of the same size.

As you can see, this function does something very specific, has probably very niche uses, and its usefulness is debatable. One could easily argue that this or that detail should work differently. In the end, implementing a model like this in terms of other functions is not too difficult, and I would rather see igraph::sample_islands() removed from igraph. To be clear, there are no plans for removal at this moment. However, I would appreciate feedback about how you use this function, whether you find it useful at all, or whether you exposed it simply because it was present in igraph.

Alternatively, if you do find use for it, we can also consider improvements. C/igraph 1.0 is imminent, so improvements which require breaking changes may be possible in the next couple of weeks, but not after. (Note that many improvements that wouldn't be considered breaking in R are in fact breaking in C.)

jhollway commented 2 months ago

Ok, I've passed it just a single integer now, and this will go into the next release (probably next week). The function generate_islands() edits the resulting object after igraph::sample_islands() anyway to make sure that the number of nodes agrees with the parameters entered by the user.

I (would) use this function to illustrate and teach community detection as well as network resilience. The {manynet} syntax is such that a user should expect to be able to specify the resulting number of nodes in a generated network, but we can continuing doing this as an 'aftermarket' feature if it would break C/igraph.

szhorvat commented 2 months ago

The {manynet} syntax is such that a user should expect to be able to specify the resulting number of nodes in a generated network, but we can continuing doing this as an 'aftermarket' feature if it would break C/igraph.

I'm not following completely, but if you have a specific improvement suggestion, I'm happy to listen (without making hard promises).

Maybe what you are looking for is something similar to sample_sbm(), but specifying connection counts (like sample_gnm) instead of connection probabilities (like sample_gnp)?

jhollway commented 2 months ago

It's fine, we have a workable solution that now interacts with igraph correctly.

The improvement suggestion would be to make sure all igraph::sample_*() functions can be specified to be any arbitrary network size/order, i.e. number of nodes. Since currently one cannot have an odd number of nodes when selecting an even number of islands, this is not currently the case for igraph::sample_islands(). We use node deletion to obtain the correct network order, but there is probably a more elegant solution.