Check that matrix sizes are in the supported range before passing them to R

[szhorvat]

Fixes #889

This fix is theoretical at the moment. I can't seem to test it on macOS. The test would use a very large amount of memory.

> g<-make_empty_graph(.Machine$integer.max + 2)
> distances(g, v=1)
Error: vector memory exhausted (limit reached?)

@krlmlr @Antonov548 Can either of you try to test this on a large-memory Linux machine? I don't have access to one at this moment. Or otherwise do we merge as-is without a through test?

[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 PR was merged using Aviator.

---

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.

[krlmlr]

Instead of a one-off test, can we instead implement a function that checks matrix dimensions, test the behavior of that function (in an automated test), and use it where needed?

If we don't want to expose this function to R, we can use the catch testing framework in testthat: https://testthat.r-lib.org/reference/use_catch.html .

[szhorvat]

Each matrix type needs a different implementation, so this can't be reduced to calling a single function three times

[krlmlr]

Just the part where ncol and nrow is checked, perhaps?

[szhorvat]

test the behavior of that function (in an automated test),

How do you intend to test a C function from R? R doesn't even support the integer type that the function would be operating with. What would be the point of testing a function that just compares two integers and reports an error? I am not sure how to proceed here.

I managed to test manually that the functionality works after setting R_MAX_VSIZE=100Gb, using the command line instead of RStudio, and using a barebones function like layout_in_circle(). The R process used over 120 GB of memory.

> g<-make_empty_graph(n=.Machine$integer.max+1)
> l<-layout_in_circle(g)
Error in layout_in_circle(g) : 
  At rinterface_extra.c:2588 : igraph returned a matrix of size 2147483648 by 2. R does not support matrices with more than 2147483647 rows or columns. Failed

[krlmlr]

This could be extracted into a function, say, R_igraph_check_matrix_dims(igraph_integer_t nrow, igraph_integer_t ncol) . This function becomes testable, e.g., through an R wrapper that we export just for testing. (Overkill, perhaps.)

We could also go one step further and make this function sensitive to a global switch (one that's perhaps only active in debug mode). The global switch would change the definition of INT_MAX for this function so that we can even test that (some) functions that return a matrix actually go through this code path, without allocating tons of memory. (Overkill, perhaps.)

But these two steps combined allow creating a one-off version of igraph that doesn't require allocating hundreds of gigabytes to check the functionality.

No action needed here, but for the future, should we strive to take testability into account when implementing?

[szhorvat]

should we strive to take testability into account when implementing

This PR fixes a bug that manifests itself in difficult to test scenarios. Nevertheless it is a bug that should be fixed. I did verify the fix manually.

This function becomes testable, e.g., through an R wrapper that we export just for testing.

The function would take arguments of type igraph_integer_t. This type is not supported by R. Using double makes things complicated and increases the chance of mistakes (not every igraph_integer_t value is representable as a double).

So I'm not sure how to do this testing in a productive way.

[krlmlr]

It's in the merge queue, thanks for working on this.

Happy to elaborate if this situation occurs on an issue from https://github.com/igraph/rigraph/milestone/14.

[krlmlr]

Thanks!