fix: subgraph_centrality() now ignores edge directions
#1414
Conversation
Current Aviator status
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.
|
|
@maelle Please have a look at how I access functions from the |
szhorvat
force-pushed
the
fix/subgraph_centrality
branch
3 times, most recently
from
2523a35 to
2f6161f
Compare
| @@ -0,0 +1,24 @@ | |||
| test_that("subgraph_centrality works", { | |||
| g <- make_graph("Frucht") | |||
There was a problem hiding this comment.
I'll make a commit to rename those to not re-use names which is a pet peeve of mine 😇 https://masalmon.eu/2024/05/23/refactoring-tests/
There was a problem hiding this comment.
I don't understand what you mean.
There was a problem hiding this comment.
In the test two objects were created with the name "g". I prefer the two names to be different.
There was a problem hiding this comment.
I see what you mean after looking at your commits, but keep in mind that there is no special significance to this being the Frucht graph here. If all you need is to not reassign g, I suggest g1 and g2 for similar situations in the future. Then the variable doesn't need to be renamed when the graph is changed.
The reason why mathematicians don't spell out variable names is not that they don't know any better. It's because sometimes it is useful to be generic and not be distracted by things that are irrelevant to the work at hand.
There was a problem hiding this comment.
Fair enough, but with g1 and g2 I'd easily get confused. They can be uninformative but they have to be really different for me 😁
The method that this function uses does not work in general for directed graphs, and the concept of subgraph centrality is normally defined for undirected graphs only.
The method that this function uses does not work in general for directed graphs, and the concept of subgraph centrality is normally defined for undirected graphs only.
Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>
aviator-app
bot
force-pushed
the
fix/subgraph_centrality
branch
from
7075ca4 to
c7cdef6
Compare
The method that this function uses does not work in general for directed graphs, and the concept of subgraph centrality is normally defined for undirected graphs only.
I consider this a bugfix, so I don't think any lifecycle work is necessary. A revdepcheck will be useful, as some users may have tried to use this for directed graphs (in which case the function wouldn't return meaningful results, as already indicated in its documentation).
Please consider including this simple fix in the next release.