Docs/polish round 1 1p0p0 - #466
Conversation
chief-dweeb
left a comment
chief-dweeb
left a comment
There was a problem hiding this comment.
Looks great - some nits in my comments.
-Fred
| One other addition worth knowing: MarkovChain() now takes an `rng` argument (an integer seed or a | ||
| `random.Random`), and the chain's RNG is passed to your proposal and acceptance functions. This is | ||
| what makes runs reproducible. There is more on this in the reproducibility guide. |
There was a problem hiding this comment.
Nit - it might be nice to just say that "RNG" stands for random number generator...
There was a problem hiding this comment.
Fair. I'll add that
| Instead of the `partial` incantation, v1.0.0 provides slim, ready-made builders that provide a | ||
| simple interface to return a proposal function directly: |
There was a problem hiding this comment.
I would not use "instead" as it implies that the old way is gone. I would also add a brief note about why we added the new (equivalent) approach - something along the lines of 1) some users found the use of "partial" hard to understand and 2) the new approach is just more readable.
| The variants differ along two axes. `district_pairs_*` picks uniformly among adjacent district | ||
| pairs, while `cut_edges_*` picks a cut edge at random and merges the districts on either side, so a | ||
| pair's chance is proportional to how many cut edges it shares. `*_mst` draws a minimum spanning tree | ||
| over random edge weights using Kruskal's algorithm; `*_ust` draws a uniform spanning tree using | ||
| Wilson's algorithm. `ReCom.reversible(...)` is Reversible ReCom. The single-letter aliases `A`, `B`, | ||
| `C`, `D`, and `R` are also available. |
There was a problem hiding this comment.
I would list the variants here before talking about them. Something like:
There have always been several ways to run ReCom, and we have provided specific variants for these:
<table with names and brief descriptions>
As you can see, these variants differ in two ways:
| > methods. Note also that the old class's `ideal_pop` parameter is spelled `pop_target` throughout | ||
| > v1.0.0. | ||
|
|
||
| When you need parameters the slim builders do not expose, use `build_recom_proposal_fn`, which takes |
| If you pass any of these by keyword, they need updating: | ||
|
|
||
| | Legacy name | v1.0.0 name | Where | | ||
| | ----------------- | ---------------------------- | -------------------------------------- | | ||
| | `method` | `bipartition_tree_fn` | `recom` | | ||
| | `balance_edge_fn` | `find_balanced_edge_cuts_fn` | `bipartition_tree` | | ||
| | `one_sided_cut` | `single_district_cut` | `bipartition_tree`, custom cut finders | | ||
| | `choice` | `cut_choice_fn` | `bipartition_tree` | |
| `MultiMemberReCom` offers the same four non-reversible variants as `ReCom`, plus | ||
| `build_multi_member_recom_proposal_fn` for the full parameter set. Three things differ from the | ||
| single-member case: | ||
|
|
||
| - `pop_target` is the population for a _single member_, so it is the total population divided by the | ||
| total number of members, not by the number of districts. | ||
| - Member counts attach to district labels and stay fixed for the whole run. The keys of | ||
| `members_per_district` must match the partition's district labels exactly, and every count must be | ||
| a positive integer. | ||
| - The equal-population constraint is not appropriate, since districts are deliberately unequal in | ||
| size. Use `within_percent_of_ideal_population_per_member` instead. | ||
|
|
||
| There is no reversible multi-member variant, and no random multi-member seed generation yet; build a | ||
| starting plan by merging single-member districts. The [ReCom user guide](../user/recom.ipynb) has a | ||
| worked example. |
| "Surcharges are best read against the random weights they compete with, which are drawn from\n", | ||
| "$[0,1)$. A surcharge of 0.1 only reorders edges whose random weights happen to land within\n", | ||
| "0.1 of each other, so it is a very weak bias. In this example, values from roughly 0.3 upward are \n", | ||
| "where the effect becomes easier to observe." |
| "\n", | ||
| "### How Member Counts Work in the Code\n", | ||
| "\n", | ||
| "A key distinction between the standard ReCom and Multi-Member ReCom is that in the Multi-Member\n", |
There was a problem hiding this comment.
Very nice explanation.
Grammar nit - you state "if you declare that districts 1 and 2 to both have 1 member..." I think you should lose the word, "that", so it reads "if you declare districts 1 and 2 to both have 1 member..."
There was a problem hiding this comment.
Thank you! And good catch; I will make that change.
| "\n", | ||
| "### Setting Up the Initial Partition\n", | ||
| "\n", | ||
| "There are two things are worth keeping separate: \n", |
There was a problem hiding this comment.
Grammar nit: instead of" "There are two things are worth keeping separate:" I think you want: "There are two things worth keeping separate: \n". That is, change "are worth" to just "worth"...
There was a problem hiding this comment.
Noted. Will fix. Thanks!
|
Great! Thank you for catching my grammar in this. I'm moving a bit fast right now, so I have not spent as much time polishing as I usually do. I'll make these changes, merge them in, update the branches, and send you an email when I have finished. Expect something within the next hour. |
2 participants
Summary
Documentation polish ahead of the 1.0.0 release, working through the review notes from PR #464 and the follow-up email thread with @chief-dweeb.
No library behavior changes. The two files touched under
gerrychain/change only a comment and docstring cross-reference roles.Changes
ReComnamespace.MarkovChainparameter renames (proposal,accept,initial_state) and corrected the section that claimed no legacy changes were needed (this will be updated again later when I get the deprecation shims in).gerrychain_demo.gifhad been overwritten with a copy of the plain ensemble gif, losing the spanning-tree and cut-edge frames.generate_recom_assets.pycan now render it.recom.ipynb.recom.ipynbrx_migration_guide.mdand a dead test helper.Testing
make docs, warnings-as-errors)make testreports 456 passed, 10 skipped, 2 xfailed.Reviewer Notes
:meth:, so the rendered API docs describednodes,edges,node_indices, andedge_indicesas callables. Fixed by properly tagging these as:attr:in the docstrings.