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

Add new balance edge algos & reversible ReCom #94

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pjrule
Copy link
Collaborator

@pjrule pjrule commented Nov 11, 2020

This PR is too big, which probably means the balance edge algorithms should live in their own file so that they can be reviewed separately. Right now, this PR contains:

  • A contraction-based balance edge algorithm ported from GerryChain by @matthewkol186
  • A memoization-based balance edge algorithm created by me
  • An implementation of reversible ReCom
  • Iterator-based APIs for ReCom and reversible ReCom
  • API changes to make various components of the chain (balance edge function, tree function, etc.) more hot-swappable

@bsuwal
Copy link
Contributor

bsuwal commented Nov 18, 2020

I agree that this PR is too big. I would like the PR to be split further and with unit tests to feel better about it. One way you could break this PR down could be;

  • I like the idea of a separate file, something like tree_partition.jl that can handle balanced cut algorithms.
  • A single reversible recom PR without using the resumable library
  • Adding the @Resumable / @yield functionality separately. or using the Base.iterator suggestion that @matthewkol186 made last week?

I do understand that this seems like a lot of work. Let's talk about this though!
Also thanks for the good function documentation!

@pjrule
Copy link
Collaborator Author

pjrule commented Nov 18, 2020

Let's split things into three files:

  • recom.jl - regular ReCom proposal and auxiliary functions.
  • revrecom.jl - reversible ReCom proposal and auxiliary functions.
  • balance.jl - algorithms for finding balanced cuts (common to both chains)

@bsuwal suggests splitting the balanced cuts code out into a separate PR.

Copy link
Collaborator

@sunnymatt sunnymatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks reasonable to me upon skimming! I see resumable functions, as per our meetings I'm guessing we're going to change that potentially?

src/recom.jl Outdated
tree_fn::Function=wilson_ust,
balance_edge_fn::Function=memoized_balance_edges,
node_repeats::Int=1)::Union{RecomProposal, String}
"""TODO"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be done! heh

InnovativeInventor added a commit that referenced this pull request Jul 26, 2021
Although this is a breaking change, this brings GerryChain Julia
more in line with GerryChain Python and allows the user to save
and export state more easily. This API is different from the one
proposed in #94 in that it returns a Partition object and the
scores.
InnovativeInventor added a commit that referenced this pull request Aug 3, 2021
* Make recom and flip chains an iterable a la GerryChain Python

Although this is a breaking change, this brings GerryChain Julia
more in line with GerryChain Python and allows the user to save
and export state more easily. This API is different from the one
proposed in #94 in that it returns a Partition object and the
scores.

* Add recom_chain and flip_chain functions for backwards compat.

The iterators are now exposed in `recom_chain_iter` and
`flip_chain_iter` respectively to avoid any breaking changes.

* Update type signatures of the iterator APIs

* Point to issue on GitHub with the bug in ResumableFunctions.jl

* Export iter API

* Add note about `Partition` being mutable and changing in-place
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.

None yet

3 participants