Closed maelle closed 5 months ago
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.
will rebase once #1297 is merged.
@szhorvat do you feel strongly about any of the test cases I removed? See my first comment.
Is the code you quote in the top post what you removed? It's a bit hard to follow what's missing since code was moved between files.
It seems to me that the tests that you quote were all relevant. Why were they removed? I'm a bit lost.
Why were they removed? I'm a bit lost.
Because I couldn't see what they brought based on the tests that are now in there, and they're a bit complicated (for instance, why is there a need to define the function called gc()
).
These tests verify that the properties that the function guarantees actually hold:
I don't mind a refactoring of the test, but all these properties should be tested, ideally for all methods, including both the undirected and directed case.
There is a real risk that a refactoring of this function will accidentally break some of these properties, so it's important to test them.
P.S. It'd be good to rename the methods in this function quite soon, see #876. The current names are rather bad and confusing, I can't even remember which properties hold for which method without looking them up ... This is why I didn't list them.
ok, I'll add the tests back. thank you!
@szhorvat in
gc <- function(graph) {
clu <- components(graph)
induced_subgraph(graph, which(clu$membership == which.max(clu$csize)))
}
what could be a more explicit name for gc? it'd help me (and future contributors I guess) understand the test.
this is nearly ready but I'd like to rename the helper function with your feedback. All tests are now in there, and they don't re-use the name "g" and "nG".
what could be a more explicit name for gc?
I'm sorry, I'm not paying attention ... "gc" refers to the giant component and actually we have a function for that now! It's called largest_component()
. The reason why it is used here is to create a degree sequence that is graphical and has a connected realization. This is done by creating a connected random graph, and taking its degrees.
Let's do g <- largest_component(sample_gnp(1000, 2 / 1000))
mmmh why was this merged before the checks were run?!
@krlmlr did the Aviator settings somehow change?
Weird, let's keep an eye on that.