Find potential matches faster #61
Conversation
Not perfectly aligned, but seems pretty good
…ints of S that match
robinmessage
force-pushed
the
rhm31-cpc-10-faster
branch
from
4d51188
to
8cda030
Compare
patricoferris
changed the title
There was a problem hiding this comment.
I did a review pass, looks good, just a few things I'd like to see changed before we merge, most "we could do this" comments could become issues if you think they're right Robin.
I must confess that whilst I did get the general idea of DRangedTree works externally, I didn't review the implementation in detail, rather how it was used.
| @@ -0,0 +1,401 @@ | |||
| import argparse | |||
There was a problem hiding this comment.
This file should replace the old find_potential_matches.py, not sit along side it
There was a problem hiding this comment.
Agree, but holding off until tmf-pipeline is updated to pick this up.
There was a problem hiding this comment.
I've asked @patricoferris to comment on what he thinks the best plan here is in terms of commit ordering
There was a problem hiding this comment.
Thanks for the thought, but just go for it, make the change, commit! I can pull arbitrary commits into arbitrary parts of the pipeline, so if I have to do SHA gymnastics to get a hold of the original potential matches codes that's ok (I already do that for other things). I'd rather the merged code have some order to it and the disorder can live in the pipeline should we need that :))
There was a problem hiding this comment.
Agreed, merging once CI agrees.
Horizontal striping roughly doubled performance, so well worth it Other changes minor
|
@mdales I think I have fixed everything or made an issue for it, if you wouldn't mind taking another look please? |
| @@ -0,0 +1,401 @@ | |||
| import argparse | |||
There was a problem hiding this comment.
I've asked @patricoferris to comment on what he thinks the best plan here is in terms of commit ordering
Run time for Gola with 18 processes:
real 2m38.122s
user 39m43.770s
sys 0m29.981s
Which is a great improvement.
CPC alignment is off, and I think the matching to k might be slightly broken; I need to test the DRangedTree with the new CPC data to check the relative widths and zero widths are handled correctly.