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

Does the implementation of other-direction need to call edge-description->edge ? #57

Open
jafingerhut opened this issue Jan 31, 2020 · 1 comment

Comments

@jafingerhut
Copy link
Contributor

The implementation of function other-direction first checks whether the edge argument returns true for (undirected-edge? edge), returning nil immediately if it returns false. The only values for which undirected-edge? returns true, at least in the current implementation, are UndirectedEdge objects, for which edge-description->edge will always return its argument.

It seems this is at most a small performance degradation, and that not calling edge-description->edge would leave function other-direction doing exactly what it does now, but slightly faster.

Then again, I may be missing some case where it is useful to call edge-description->edge from other-direction

@Engelberg
Copy link
Owner

I think you're right that the call to edge-description->edge is unnecessary. I was probably thinking that since the call to undirected-edge? is a protocol that can theoretically be extended to other data structures, it was necessary. However, edge? is currently hardcoded to test for Edge or UndirectedEdge objects, which means edge-description->edge wouldn't know what to do with another kind of edge.

So it seems like either the call to edge-description->edge should be removed, or perhaps the first branch of edge-description->edge's cond should be modified to (or (directed-edge? ed) (undirected-edge? ed)) to better respect the protocols. It's hard to know whether such a modification would be useful, because I don't currently have a good example use case for overriding these protocols. Without a good test case, it's likely there are some other spots where there are baked-in assumptions that edges can only be Edge or UndirectedEdge objects. I imagined that one day I would build a disk-backed version of ubergraph for graphs too large for memory, which is why some of those protocols exist.

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

No branches or pull requests

2 participants