search

petrelharp / ftprime_ms

4 stars 2 forks source link

Updated to use msprime 0.6.0 APIs. #85

Closed jeromekelleher closed 5 years ago

molpopgen commented 5 years ago

This looks good. The only thing I can think of is if you want to introduce the new segment overlapper method instead of the heap queue approach, as it is easier to understand. The argument against that is that the queue is what is described in the paper.

petrelharp commented 5 years ago

Looks good. I think it's better to keep it matching the paper, since the main point of this is illustrating the ideas in the paper. Is this ready to merge, @jeromekelleher ?

jeromekelleher commented 5 years ago

This looks good. The only thing I can think of is if you want to introduce the new segment overlapper method instead of the heap queue approach, as it is easier to understand. The argument against that is that the queue is what is described in the paper.

I agree it would be better to use the segment overlapper, but the code is here to illustrate the algorithm in the paper, so I think it should stay as-is. The alternative is to update the algorithm, which I don't think any of us want to take on.

jeromekelleher commented 5 years ago

Looks good. I think it's better to keep it matching the paper, since the main point of this is illustrating the ideas in the paper. Is this ready to merge, @jeromekelleher ?

Yep, merge away whenever suits.