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

Simplify and document expansion functions of Graph #413

Merged
merged 1 commit into from
Jul 28, 2024

Conversation

patowen
Copy link
Collaborator

@patowen patowen commented Jun 30, 2024

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:

  • The term "parent" no longer depends on the graph's generation order. It now refers to the node along the first descender in enum order.
  • Uses of the term "parent" and "child" were removed whenever they didn't refer to the above meaning of "parent". This includes renaming insert_child to insert_neighbor and renaming populate_shorter_neighbors_of_child to populate_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).
  • An unused code path was removed in ensure_neighbor to cover the impossible scenario of a not-yet-generated shorter neighbor. Removing this code-path turned ensure_neighbor into a one-liner.
  • The most conceptually difficult function in the algorithm, 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 for insert_neighbor).

@patowen patowen requested a review from Ralith June 30, 2024 03:39
@patowen
Copy link
Collaborator Author

patowen commented Jun 30, 2024

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.

common/src/graph.rs Outdated Show resolved Hide resolved
common/src/graph.rs Outdated Show resolved Hide resolved
common/src/graph.rs Show resolved Hide resolved
common/src/graph.rs Show resolved Hide resolved
common/src/graph.rs Outdated Show resolved Hide resolved
@Ralith
Copy link
Owner

Ralith commented Jul 27, 2024

Great improvement overall! I don't think I ever actually understood this logic, so this clarity is striking.

@patowen
Copy link
Collaborator Author

patowen commented Jul 28, 2024

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:

  • At the top of 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.
  • In insert_neighbor, I renamed shorter_neighbors_of_neighbortoshorter_neighbors_of_subject`.
  • I changed the name of the function populate_shorter_neighbors_of_neighbor with populate_shorter_neighbors_of_subject and updated the function header comment to explain what "subject" means.
  • I removed the comment at the top of the populate_shorter_neighbors_of_subject function explaining the use of "subject", as the concept is no longer introduced there.
  • I added the following addendum to 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.
  • I used shorter_neighbors_of_subject.into_iter().flatten() as suggested.

@patowen
Copy link
Collaborator Author

patowen commented Jul 28, 2024

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.

@Ralith Ralith merged commit 6ac2524 into Ralith:master Jul 28, 2024
4 checks passed
@patowen patowen deleted the simplify-graph branch July 28, 2024 19:09
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.

2 participants