Fixing union vignette

[mufernando]

Replacing #200

[petrelharp]

Thanks - this works for me. (To get it to actually re-run I have to do:

rm -rf _build && rm {A,B,C,I,root}.trees && make

I spent some time trying to change the reloading block to late() instead of early(); however, this results in an off-by-one-generation error when unioning I and C together, strangely. I'm sure it'd make sense eventually. In the process I rewrote the recursion to be more pythonic (I think) and hopefully simpler - your version was very R-like.

[codecov-commenter]

Codecov Report

Merging #201 (7dd0faa) into main (5478b47) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #201   +/-   ##
=======================================
  Coverage   88.40%   88.40%           
=======================================
  Files           8        8           
  Lines        1035     1035           
  Branches      198      198           
=======================================
  Hits          915      915           
  Misses         88       88           
  Partials       32       32           

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5478b47...7dd0faa. Read the comment docs.

[petrelharp]

Have a look - I think this is ready to merge. Thanks!!

[mufernando]

Yeah, that off by one error is annoying. I fixed it once but then the SLiM script looks uglier - you'd have to treat the root differently from all the other branches.

This looks good to me. I prefer to have merged be a parameter to the union_children function, though. It can be confusing to have a function modify a random object in the environment.

[mufernando]

Sorry, I wasn't clear: I think you can merge this, but I never know how to best do the recursion when we are modifying an object in the environment. Just raised this to hear your thoughts. It seems there is no consensus on this: https://stackoverflow.com/questions/4331006/persistent-objects-in-recursive-python-functions

[petrelharp]

I'm with you in general. I'm not sure what's the best way to do it here, though. Probably the neatest way would be to make a class that has the dictionary as internal state, and the recursive function would be a method of it. That'd make it more clear. But also would be too advanced for this.

Even better - we don't actually need to union our way up the tree - we could union in any order as long as we could figure out what the appropriate split time was. I haven't thought of a particularly clean way to do that, though, so I say we just leave it as-is.

[petrelharp]

I'll rebase and merge this.