mggg / GerryChain

Use MCMC to analyze districting plans and gerrymanders
https://mggg.github.io/GerryChain/
Other
132 stars 74 forks source link

node_repeats Ignored/Unused in bipartition_tree_random #404

Closed mkarrmann closed 1 year ago

mkarrmann commented 2 years ago

The node_repeats parameter of bipartition_tree_random is documented as A parameter for the algorithm: how many different choices of root to use before drawing a new spanning tree.. However, in fact this parameter has no effect, because it is unused.

The simplest way to observe this is to look at the code and observe that node_repeats isn't used:

https://github.com/mggg/GerryChain/blob/3179ac1a4a072377e0d61b4de796cbe5985d5803/gerrychain/tree.py#L224-L257

Instead, a new spanning tree is generated on each attempt (the equivalent of the expected behavior of node_repeats=1).

The solution is to simply implement the logic for tracking the number of restarts that is used in the non-randomized bipartition_tree:

https://github.com/mggg/GerryChain/blob/3179ac1a4a072377e0d61b4de796cbe5985d5803/gerrychain/tree.py#L206-L219

mkarrmann commented 2 years ago

I just tried pushing a fix but apparently I don't have permission to push a branch.

mkarrmann commented 2 years ago

Could I please be added as Collaborator (or some equivalent) to the repo to push this fix?

This is a very straightforward bug with a simple fix that I ran into when playing around with Gerrychain.

Pinging @InnovativeInventor since you seem to be most active

pjrule commented 2 years ago

Hi, @mkarrmann! Thanks for your contribution. We generally accept third-party contributions via pull requests—would you mind submitting one?

mkarrmann commented 2 years ago

@pjrule Thanks for the reply. I don't seem to have permission to push to the repo, so I'm unable to raise a PR. I assume I need to be added as a Contributor but let me know if I'm probably just overlooking something!

mkarrmann commented 2 years ago

I did find out that you can open PRs from forks, is that what you mean?

pjrule commented 2 years ago

Yes! :) Sorry for the overhead.

On Nov 15, 2022, at 12:15 AM, Matt Karrmann @.***> wrote:

I did find out that you can open PRs from forks, is that what you mean?

— Reply to this email directly, view it on GitHub https://github.com/mggg/GerryChain/issues/404#issuecomment-1314786848, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJSIG7YQL2TQWWEPCQXBNLWIML5JANCNFSM55NKVDYQ. You are receiving this because you were mentioned.

mkarrmann commented 2 years ago

No worries!