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

Test centrality with generic graph #272

Merged

Conversation

simonschoelly
Copy link
Contributor

@simonschoelly simonschoelly commented Jul 2, 2023

This PR changes the centrality algorithms so that they test with GenericGraph instead of SimpleGraph.

Some tests did fail, because we passed the output of vertices(g) to degree - but previously that function would not only accept an AbstractVector and not a generic collection of vertices, so I changed indegree, outdegree and degree, so that they can take an arbitrary collection of vertices. Maybe in the future, we could also just demand, that vertices(g) returns an AbstractVector.

Furthermore there was an issue with betweenness_centrality where we tried to index into vertices(g) - this also would have been a non-issue, if vertices(g) was an AbstractVector.

Lastly, I had to change the input in the sample method, that could also not deal with the output of vertices(g).

@simonschoelly simonschoelly added the do not merge Do not merge this PR (yet) label Jul 2, 2023
@simonschoelly simonschoelly self-assigned this Jul 2, 2023
@codecov
Copy link

codecov bot commented Jul 2, 2023

Codecov Report

Merging #272 (624d9e6) into master (745add6) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #272      +/-   ##
==========================================
+ Coverage   97.19%   97.28%   +0.09%     
==========================================
  Files         114      114              
  Lines        6656     6657       +1     
==========================================
+ Hits         6469     6476       +7     
+ Misses        187      181       -6     

@gdalle
Copy link
Member

gdalle commented Jul 2, 2023

Partial solution for #224

@simonschoelly simonschoelly force-pushed the test-centrality-with-generic-graph branch from f3dce5d to c8bcea3 Compare July 2, 2023 21:06
@simonschoelly simonschoelly removed the do not merge Do not merge this PR (yet) label Jul 2, 2023
@simonschoelly simonschoelly mentioned this pull request Jun 29, 2023
12 tasks
@gdalle gdalle added the enhancement New feature or request label Jul 3, 2023
@simonschoelly simonschoelly force-pushed the test-centrality-with-generic-graph branch from c8bcea3 to 624d9e6 Compare July 4, 2023 22:20
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.

Good catches, once again!

src/core.jl Show resolved Hide resolved
@gdalle gdalle merged commit 2760c27 into JuliaGraphs:master Jul 5, 2023
jwassmer pushed a commit to jwassmer/Graphs.jl that referenced this pull request Jul 5, 2023
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
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants