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

Algorithms: rework shortest_paths #594

Merged
merged 127 commits into from
Dec 29, 2023
Merged

Algorithms: rework shortest_paths #594

merged 127 commits into from
Dec 29, 2023

Conversation

indietyp
Copy link
Member

@indietyp indietyp commented Oct 20, 2023

This PR reworks all shortest path algorithms into one cohesive set of structs (instead of functions)

This means instead of having separate functions; we have: ShortestPath<S> and ShortestDistance<S>, where ShortestDistance<S> only records the distance, not the path it took to get there.

Every one of them has four functions (three of them being auto-implemented):

  • *_from: Compute all (shortest) paths from a specific source node to all nodes
  • *_to: Compute all (shortest) paths from all nodes from a specific target node
  • *_between: Compute the shortest path between a source and a target node
  • every_*: compute all (shortest) paths between nodes x nodes (so A -> B, A -> A, B -> A, B -> B, cartesian product)

The prefix/suffix is path or distance to guard against likely method clashes, which would necessitate explicitly specifying the trait.

Why is this useful? Composition. Different algorithms (I intend to do something similar for other categories of algorithms) have pros/cons depending on the graph they are given; for example, dijkstra only works with positive weights but is faster than bellman-ford. Let's say I have a function/algorithm that needs the shortest path between two nodes instead of deciding which algorithm to use and which cost function to use, etc. What I can do is have an argument that takes a generic: ShortestPath<S> or ShortestDistance<S>, and the user can decide what they want (optionally, one can provide some default implementation). I hope. This super powerful concept will spark innovation and build on existing algorithms.

The other change is that all functions above return iterators. The reason for this is simple: lazy computation and great flexibility. Imagine you want to get all the shortest paths from A, but I am only interested in the lowest of the first five nodes or the first five nearest nodes (we might want to guarantee ordering here, if possible?) Instead of eagerly evaluating (which would never terminate in an infinite graph), one could evaluate the iterator five times and use any of the combinators. That's a big win. We no longer need to define the shortest path for every case; instead, we can use combinators to do the job for us. That is quite a powerful concept.

A function that makes use of this concept can be seen in the test code:

fn path_from<'a, S, P>(
    graph: &'a Graph<S>,
    source: &'a S::NodeId,
    algorithm: &P,
) -> HashMap<S::NodeId, (P::Cost, Vec<Node<'a, S>>)>
where
    P: ShortestPath<S>,
    S: GraphStorage,
    S::NodeId: Copy + Eq + Hash,
{
    algorithm
        .path_from(graph, source)
        .unwrap()
        .map(|route| {
            (
                *route.path.target.id(),
                (route.distance.value, route. path.to_vec()),
            )
        })
        .collect()
}

This shows that we do not compromise readability or add any significant complexity!

@indietyp indietyp marked this pull request as ready for review December 28, 2023 18:46
@indietyp
Copy link
Member Author

Sadly, this took quite a while (sadly), but many things happened in my (personal) life, and this hobby project needed to take a backseat. Thank you to @thehabbos007 for the initial SPFA implementation!

Hopefully, with the foundation of how the algorithms are supposed to work, things should speed up.

Note: Dijkstra and some other algorithms currently have a speed regression. I already have other branches in the works to solve this problem (which is why this also took a bit longer).

Considering the other PRs I will likely keep this open until tomorrow and then merge so I can continue work.

Next up are:

  • algorithm improvements through GraphId dependent storage
  • GraphId requirements (add Copy, Debug, Display) Copy is exciting as an experiment to see the impact it has on algorithms
  • rework of GraphStorage::from_parts
  • proptest
  • traversal
  • views
  • ...

I have decided that one of the last things will be other graph implementations, just so that everything could settle in properly.

Hopefully 🤞 these should be faster and more contained now that we have more of the framework up and running. There were still quite a bit of open questions and documentation is like always one of the bigger hurdles, but I don't want to delay creating it, because we'll forget.

@indietyp
Copy link
Member Author

I am aware of the large diff, snapshots will be uploaded to Git LFS instead

@indietyp indietyp merged commit 71839d6 into next Dec 29, 2023
1 of 5 checks passed
@indietyp indietyp deleted the bm/algorithms-shortest-path branch December 29, 2023 10:12
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.

None yet

2 participants