Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added edge-betweenness.jl to centralities #277

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

jwassmer
Copy link

@jwassmer jwassmer commented Jul 4, 2023

I have added a new file edge-betweenness.jl to src/centrality/. Here I include a function to compute the edge betweenness of a graph (directed and weighted). In theory I could also add a version for MultiGraphs, but these are not in the base version of Graphs atm. My code is based on the version from networkX.

This is my first contribution, and I hope I have followed all the guidelines correctly.

Jonas

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #277 (a4016b7) into master (d3b2706) will decrease coverage by 0.02%.
The diff coverage is 94.28%.

❗ Current head a4016b7 differs from pull request most recent head 921d837. Consider uploading reports for the commit 921d837 to get more accurate results

@@            Coverage Diff             @@
##           master     #277      +/-   ##
==========================================
- Coverage   97.29%   97.28%   -0.02%     
==========================================
  Files         114      115       +1     
  Lines        6659     6694      +35     
==========================================
+ Hits         6479     6512      +33     
- Misses        180      182       +2     

@simonschoelly
Copy link
Member

Hi Jonas, thanks for your contribution.

Code reviews here take quite a while at the moment, especially when the reviewer tries to understand the algorithm and we are also a bit understaffed, just to warn you :)

That being said, the first thing that you definitely should also implement are some tests - that is why the codecov bot is complaining so loudly. If you want to use them, we recently introduced some graph types called GenericGraph and GenericDiGraph - with these graph types we should be able to catch some errors that we might not see if we use SimpleGraph or SimpleDiGraph - this has not been changed in a lot of the existing tests yet, but here is an open PR for the other centrality algorithms if you want to look how it is done there: #272

@gdalle gdalle added enhancement New feature or request do not merge Do not merge this PR (yet) labels Jul 5, 2023
@simonschoelly simonschoelly removed the do not merge Do not merge this PR (yet) label Jul 5, 2023
@simonschoelly
Copy link
Member

You may want to add the edge_betweenness_centrality symbol to the list here so that it is made available when someone writes using Graphs.

Furthermore, you actually need to include the src/centrality/edge-betweenness.jl file, otherwise the code there is never run. That should be done in the list here.

Lastly, you also need to add test/centrality/edge-betweenness.jl file to be included in the tests that are being run when someone executes ]test Graphs]. That should be done here.

Copy link
Member

@gdalle gdalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution! I'll try to review it, and have asked a few questions to understand the procedure a bit better

to get an estimate of the edge betweenness centrality. Including more nodes yields better more accurate estimates.
Return a Sparse Matrix representing the centrality calculated for each edge in `g`.
It is defined as the sum of the fraction of all-pairs shortest paths that pass through `e`
``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you checked how this displays by building the docs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pkg> activate docs
julia> include("docs/make.jl")

`normalize=true` : If set to true, the edge betweenness values will be normalized by the total number of possible distinct paths between all pairs of nodes in the graph.
For undirected graphs, the normalization factor is calculated as ``2 / (|V|(|V|-1))``, where |V| is the number of vertices. For directed graphs, the normalization factor
is calculated as ``1 / (|V|(|V|-1))``.
`vs=vertices(g)`: A subset of nodes in the graph g for which the edge betweenness centrality is to be estimated. By including more nodes in this subset,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the relation between k and vs?

src/centrality/edge-betweenness.jl Show resolved Hide resolved
while length(seen) > 0
w = pop!(seen)

coeff = (1.0 + δ[w]) / σ[w]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain the dynamics of coeff and δ through this loop?



### References
- Brandes 2001 & Brandes 2008
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you give more details to help us check the algorithm?

@@ -0,0 +1,81 @@

@testset "Edge Betweenness" begin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did you pick your test cases?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants