Closed patowen closed 3 months ago
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 master
that were included. I'll list out the changes I made:
insert_neighbor
, I added the comment
// 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.
insert_neighbor, I renamed
shorter_neighbors_of_neighborto
shorter_neighbors_of_subject`.populate_shorter_neighbors_of_neighbor
with populate_shorter_neighbors_of_subject
and updated the function header comment to explain what "subject" means.populate_shorter_neighbors_of_subject
function explaining the use of "subject", as the concept is no longer introduced there.populate_shorter_neighbors_of_subject
's long comment:
// 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.
shorter_neighbors_of_subject.into_iter().flatten()
as suggested.I realize that my note about calling ensure_neighbor
instead of neighbor
was unnecessarily verbose for what is already a long and dense explanation, so I replaced it with the (and recursively create if needed)
parenthetical, which accomplishes the same thing in far fewer words.
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
).