-
Notifications
You must be signed in to change notification settings - Fork 20
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
Simplify and document expansion functions of Graph #413
Conversation
I believe some of the comments I've written don't make sense, although it's hard to tell in what way they don't make sense and how to improve them, so I hope that peer review will help here. |
Great improvement overall! I don't think I ever actually understood this logic, so this clarity is striking. |
Ah, the "Compare" button isn't as useful this time because it also includes all the changes from
// To help improve readability, we use the term "subject" to refer to the not-yet-created neighbor node, since the term.
// "neighbor" is already used in many other places.
// Note that this shorter neighbor might not yet be in the graph, which is why we call `ensure_neighbor`
// here instead of `neighbor`. This means that nodes in the graph will be recursively created as necessary.
|
I realize that my note about calling |
This PR accomplishes a few things, all in the spirit of hopefully reducing the mental load of understanding all the algorithms that allow
Graph
to grow:insert_child
toinsert_neighbor
and renamingpopulate_shorter_neighbors_of_child
topopulate_shorter_neighbors_of_subject
(where "subject" is a term that was introduced internally for "the neighbor node we want to insert" to try to improve clarity).ensure_neighbor
to cover the impossible scenario of a not-yet-generated shorter neighbor. Removing this code-path turnedensure_neighbor
into a one-liner.populate_shorter_neighbors_of_subject
is now documented in detail. For simplification purposes, it was also altered to return all shorter neighbors, including the passed in argument (as it previously excluded this argument, resulting in extra logic forinsert_neighbor
).