Conversation
Current Aviator status
This PR is currently in state
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.
|
| expect_identical(colnames(I2), c("h", "i", "j", "k", "l")) | ||
| }) | ||
|
|
||
| test_that("as_biadjacency_matrix() works -- dense + attribute", { |
There was a problem hiding this comment.
The C core will not support adjacency matrices that contain other values than numbers. This means that we either need to drop support for character adjacency matrices, or we need to implement this behaviour in pure R while making sure to match the C core.
There are several things that don't make much sense with non-numerical adjacency matrices. For example, what do you do with multi-edges? How do you combine their character properties? For plain adjacency matrices, what do you do with self-loops in undirected graphs (whose value is normally doubled in the matrix)?
I'm sure we had a discussion about this in the past, and even reached out to someone who used character matrices (very few people do). But I don't remember the decision. We need to search the issue tracker.
There was a problem hiding this comment.
Having the string "0" for non-existent edges makes no sense to make. Numbers should be numbers and strings should be strings—R is not Perl! 😆
If we really want to support string matrices, wouldn't "" make a lot more sense?
The question of what is the "null strings" also ties into what w do with self-loops and multi-edges. If the multi-edge combiner operation is string concatenation, then the null string is "".
These complications illustrate why I don't particularly like the idea of supporting string matrices.
There was a problem hiding this comment.
I suggest aiming to remove string matrix support, and not producing characterization tests for it. Issue: #1542
There was a problem hiding this comment.
@schochastics for info if you touch these test files too
|
See also #1540 |
| }) | ||
|
|
||
| test_that("as_biadjacency_matrix() works -- dense", { | ||
| I <- matrix(sample(0:1, 35, replace = TRUE, prob = c(3, 1)), ncol = 5) |
There was a problem hiding this comment.
Minor nitpick: Let's not call this I, as that's the standard name for the identity matrix. (It was probably called I because of "incidence" matrix.)
There was a problem hiding this comment.
and good to merge once I fix that?
There was a problem hiding this comment.
This part is good, and this is just an aesthetic nitpick that doesn't necessarily need to be fixed. My main comments are in the other thread in this PR, addressing string matrices. Will open an issue for that soon.
3 participants
No description provided.