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

WIP: start implementing Undirected module #58

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tcoopman
Copy link

@tcoopman tcoopman commented Sep 26, 2022

As a proof of concept, I've started to implement some Undirected functions (see #39). I've did it by copying all things over from Directed and changing some parts. This is not optimal.

If you feel like this is useful, I might add more functions.

@bitwalker
Copy link
Owner

@tcoopman Thanks for contributing! There are really two options - if we want to consolidate implementations for some graph operations, then we can parameterize them on their directionality and move them to a common implementation module, e.g. something like Graph.Common or Graph.Base. The alternative is to do as you have started to do, and simply copy the implementations from the Graph.Directed module to Graph.Undirected, which is fine, but as you've pointed out, isn't ideal. Would you be interested in tackling the first approach instead? I suspect that for many operations we can simply add an additional argument that is either :directed or :undirected, and then dispatch on that if the behavior differs. For operations where the behavior has little overlap, then defining those functions in Graph.Undirected makes sense to me.

In either case, I appreciate the contribution, so take whichever approach is preferable for you at this point, and if need be it can always be refactored later.

@tcoopman
Copy link
Author

I will maybe pick it up at some point again, but I currently don't have the time for it.

I like the idea of a common graph module and operations with directed and undirected as arguments for the functions. Although I'm not sure if that should be exposed as the graph type is already directed or undirected.

I'm also not sure if all functions should have an implementation for both types. For example reachable_neighbors should probably be specified differently for undirected graphs, because you always have a path back.

@dunyakirkali
Copy link

dunyakirkali commented Dec 27, 2023

@bitwalker is this what you had in mind?

A common module which delegates function to directed or undirected?

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

Successfully merging this pull request may close these issues.

3 participants