petrelharp / ftprime_ms

4 stars 2 forks source link

High-ish level comments #22

Closed ashander closed 6 years ago

ashander commented 6 years ago

I like the organization.

(I need to add some comments to Results and Discussion still)

ashander commented 6 years ago

I will be adding a touch more shortly, @petrelharp but I think this gets at my major structural comments/suggestions

ashander commented 6 years ago

ok I've added everything.

petrelharp commented 6 years ago

I maybe heavily edited these edits. Didn't address the question of Alg F; will think again later. Any other opinions?

ashander commented 6 years ago

@molpopgen @jeromekelleher any thoughts on:current state of this? especially

jeromekelleher commented 6 years ago

Regarding Alg F, I'm not sure I see the need for it. Adding a simplification step to Alg W really is very simple, and we could just explain this in the text (see this implementation for example). Basically, all we have to do when we want to simplify is (a) sort the tables; (b) call simplify with samples=P; and (c) replace the current population P with [0, 1, ..., N-1].

I've avoiding adding table objects to Alg W as I thought it would be simpler to use lists/sets. But we could easily change Alg W to record directly into node and edge tables, and then describing this extra step of simplification is trivial. We could even add this step in to Alg W I guess.

jeromekelleher commented 6 years ago

I've made a quick stab at updating Alg W over in #24 to use the table notation. Under this formulation we can easily include calls to sort_tables() and simplify. We shouldn't need the proposed Alg F at all then, as we have a fully detailed implementation of the method in one place.

Thoughts?

petrelharp commented 6 years ago

I like this solution. If you do, too, @ashander, go ahead and merge jerome's branch into here?

ashander commented 6 years ago

This is great @jeromekelleher and I think will make things clearer. I pulled in your changes to alg w and left off all the unrelated whitespace changes from #24

petrelharp commented 6 years ago

lmk when you're good for me to merge.

ashander commented 6 years ago

ok. good to merge whenever