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

HostEnvironmentDirectedGraph::Iterator #1273

Merged
merged 1 commit into from
Jan 21, 2025
Merged

HostEnvironmentDirectedGraph::Iterator #1273

merged 1 commit into from
Jan 21, 2025

Conversation

Robadob
Copy link
Member

@Robadob Robadob commented Jan 18, 2025

Branch named host_graph_traverse because I incorrectly thought this feature request was tree traversal for some reason, and created the branch before re-reading the issue.

Required some modifications to HostEnvironmentDirectedGraph::EdgeMap::Edge and CUDAEnvironmentDirectedGraphBuffers, as these were previously heavily written for Edges to be referred to by src/dest, rather than index, despite this requiring the index to be looked up by most methods.

Reason this probably wasn't implemented initially is because the index of an edge is not stable, as the edge list is sorted each time the map is built. Likewise, it iterates the buffer of allocated edges, rather than the named EdgeMap so the iterator does not match expectations of a map iterator (e.g. doesn't return key-value pairs) and leads to some ambiguity (e.g. does size() return the length of the allocated buffer, or the number of valid edges?).

Closes #1237

@Robadob Robadob self-assigned this Jan 18, 2025
@Robadob Robadob marked this pull request as ready for review January 19, 2025 13:21
This necessitated updating Edge/Vertex classes to internally track via index rather than ID.
This removed redundant index lookups from every accessor, so will have negligbly improved performance.

EdgeMap and VertexMap are now franken datastructures, which have attributes of both maps and arrays.
This is great for usability, but has the potential to confuse users that don't read documentation.

Tests extended in C & Python to cover this.

Closes #1237
@Robadob Robadob force-pushed the host_graph_traverse branch from 240261a to d0546e0 Compare January 19, 2025 13:24
Copy link
Member

@ptheywood ptheywood left a comment

Choose a reason for hiding this comment

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

TestEnvironmentDirectedGraph tests for c++ and python all pass under linux.

Changes look sensible to me, no API breaks I spotted (and we're not concerned with ABI).

Will let @mondus have the final say.

@mondus mondus merged commit c8407d7 into master Jan 21, 2025
18 of 22 checks passed
@mondus mondus deleted the host_graph_traverse branch January 21, 2025 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FeatureReq]: Host function iteration over directed graph edges
3 participants