-
Notifications
You must be signed in to change notification settings - Fork 3
Implement edges function for IGraph #26
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
base: master
Are you sure you want to change the base?
Implement edges function for IGraph #26
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #26 +/- ##
=========================================
- Coverage 6.47% 5.27% -1.20%
=========================================
Files 8 8
Lines 4342 4323 -19
=========================================
- Hits 281 228 -53
- Misses 4061 4095 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Krastanov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty impressive for jumping into it without having had prior exposure to any of these tools, good job! I think there is a much more efficient way to set it up though, check out the link I shared. And we need to add tests.
| function Graphs.edges(g::IGraph) | ||
| edge_list = Vector{Graphs.edgetype(g)}() | ||
| adjlist = IGAdjList() | ||
| LibIGraph.adjlist_init(g,adjlist,LibIGraph.IGRAPH_ALL,LibIGraph.IGRAPH_LOOPS_TWICE,LibIGraph.IGRAPH_MULTIPLE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find! This seems like a pretty expensive allocating operation. I would expect there to be a way to iterate over edges without having to create the adjlist.
I suspect this will be a better way: https://igraph.org/c/html/latest/igraph-Iterators.html#edge-iterators
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and we also need correctness tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean by correctness tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new feature usually comes with additions to the test suite, so that one can attest that the feature is correctly implemented. More importantly, such tests will be executed for any change in the future, ensuring that the feature is not accidentally broken.
For instance, here you are implementing an edges function. A convenient correctness test would be some self-consistency check. E.g. create a graph by specifically adding some random set of edges. Then call the edges function on that graph and verify that it returns the same set as the initial set you started with. This type of "round trip" consistency checks are a powerful way to detect issues.
|
the failing tests, as you guessed in your email, are unrelated to your work -- these are just tests that have not been properly set yet |
No description provided.