graph/...: use int64 IDs #70
Conversation
graph/community/louvain_directed.go
Outdated
| @@ -353,25 +352,25 @@ func (g *ReducedDirected) To(v graph.Node) []graph.Node { | |||
| func (g *ReducedDirected) HasEdgeBetween(x, y graph.Node) bool { | |||
| xid := x.ID() | |||
| yid := y.ID() | |||
| if xid == yid { | |||
| if xid == yid || xid != int64(int(xid)) || yid != int64(int(yid)) { | |||
There was a problem hiding this comment.
It seems to be rather involved to keep the community node as an int rather than an int64. May I ask, what is the main rationale for keeping it int than updating it to be consistent with node ID as int64, as is done elsewhere. This change will inevitably introduce API breakage. Might as well do one swooping change to int64, and keep the code simple and consistent?
There was a problem hiding this comment.
The communities are internal representations with dense IDs stored in memory; this means that an int is large enough. It has additional benefits performance-wise in that we are dealing with machine word-sized values on all archs instead of pushing int64s through 386 and arm. These are the people we are making the int->int64 ID change for. In addition, the code changes required for this approach are far less wide-reaching than the straight out mechanical change.
There was a problem hiding this comment.
I agree on an intuitive level that keeping the IDs in a machine word-sized value may have performance benefits for 32-bit platforms.
At the same time, without doing a benchmark on code using communities, we won't know for sure if keeping the node ID an int or changing it to int64 actually has a huge impact. There may be other parts of the code which dominate the CPU profiles, such that the ID being in int or int64 may not make a difference performance wise.
From my point of view, if the performance difference as measured by benchmarks and profiling is negligible, then focusing on readability and maintainability is more important.
Still, if there is an actually difference in measured performance when changing from int to int64, then I agree that it may be worth while to keep the internal community node IDs as ints.
There was a problem hiding this comment.
There are three things here. One is that the changes associated with moving from int to int64 are not at all clearly cleaner or more readable (this was my first motivation for limiting the change - I did actually go through and do the complete transformation first and decided it was not the way to go) and the second is that there is an immediate ~10% improvement in performance available as I can partially revert the removal of intsets, which clearly did show an improvement in this context. The last is safety, community graphs are held in slices, so the appropriate ID type is an int internally.
I will, however, add an isValidCommunityID to replace the x = int64(int(x)) checks - adding sign checks which I don't yet have.
|
@mewmew ping |
Also rename CyclesIn to DirectedCyclesIn to allow for the undirected case to be added.
The internal representation of reduced graphs remains based around int IDs since the operations must happen in memory, and there is an initial ID densification step to get from the target graph to the communities.
I didn't think about this, but I guess you are right. Since slices are using int I have looked through the code roughly, and from my perspective it looks good. I also trust your judgement on making these decisions as you have more experience with working with these graphs, and the environments that may have specific requirements or constraints (e.g. 32-bit environments). My main motivation was to make the graph API more future proof, before it would be finalized. Didn't really think about the more tricky aspects of the change, as you definitely seem to have done. I wish I could give it a more thorough review of the change, but I believe someone with experience of using the topo and community packages would be better suited for finding errors (other than trivial ones that I may locate). That being said, I know I'll play around a lot with topo this autumn, especially when experimenting with connected components in graphs, so at that point, I'll make sure to report anything I may bump into that could be strange. So, at least from me you have a LGTM to merge. @vladimir-ch perhaps you may wish to take a quick look before the merge? |
|
ping |
|
Thanks. |
Two commits that deserve careful review are "graph/topo: update for int64 IDs" (the Bron-Kerbosch routine) and "graph/community: update for int64 IDs" which is a little subtle.
@vladimir-ch @mewmew Please take a look.
After #68.
Updates #31.
Fixes #69.
Updates gonum/graph#197.
Closes gonum/graph#114.