added code for lifting over assemblies #574
Conversation
Codecov Report
@@ Coverage Diff @@
## master #574 +/- ##
==========================================
+ Coverage 99.65% 99.67% +0.01%
==========================================
Files 33 33
Lines 2328 2158 -170
Branches 293 256 -37
==========================================
- Hits 2320 2151 -169
Misses 3 3
+ Partials 5 4 -1
Continue to review full report at Codecov.
|
|
Nice one, thanks Jeff! I'm a noob when it comes to lifting over, so I have some questions about the nature of this liftOver tool that maybe are a bit offtopic, but here goes anyway...
|
|
Thanks @jradrion, this is great! @jeffspence - it would be super helpful if we could get your thoughts on what we're doing here. What we need to do is lift over older genetic maps to GRCh38 (for humans), and to whatever genome build is used in Ensembl for other species. As such, it's something we do rarely so the code doesn't need to be especially polished or automated, but we'd like to have some assurance that we've not introduced artifacts into the maps. Any thoughts would be much appreciated. |
|
@grahamgower thanks!
I'm not a lawyer, but I would argue that this does not constitute a derivative work. All I'm doing is running the UCSC liftOver tool "as is", with some pre- and post-processing going on to convert the file formats for our use. To me this is no different than running liftOver using a shell script, which I'd have to imagine would not break the EULA. If this is a concern for people it would be pretty easy to strip out the call to liftOver and split this code into two scripts, where we run the first half, then we run liftover, then we run the second half.
I'm also not a liftOver expert, and this is a really good question. I guess I've always assumed it uses some kind of hash function to just grab the coordinates from the chain file. To be honest, I have no idea how it works.
Chain files are better described here, but it's basically just a file of coordinates from genome-wide local alignments mapping between the two assemblies. The algorithm for building the chains is published (https://www.pnas.org/content/100/20/11484.full). This page describes how to make your own. |
Let's not worry about this - we're not distributing the maintenance code to end users, so I don't think we need to worry about license issues. |
|
This looks great! If I understand correctly -- you map the hapmap format file to a BED, use liftOver to remap the BED and then do a bit of post-processing (filling in gaps, and averaging within windows) to clean up the bed and map back to hapmap. My only minor quibble is on line 140 you use the mean of the rates to fill in short gaps. I think it would make a bit more sense to use the mean rate instead of the mean of the rates (the mean of the rates up-weights short intervals and down-weights long intervals; in species with hotspots, high rates will tend to be in short intervals and low rates will tend to be in long intervals, so my guess is that the mean of the rates would be a bit higher than the mean rate). This overall approach is a little bit different than what Adam Auton did to lift the hapmap maps over. This is from the README from the GRCh37 LiftOver of the HapMap recombination maps(ftp://ftp-trace.ncbi.nih.gov/1000genomes/ftp/technical/working/20110106_recombination_hotspots/README_hapmapII_GRCh37_map):
I'm not 100% sure what this actually means. It sounds like he might have looked at the actual position in genetic distance for each line in the file, and then back-calculated the rate (from the differences in physical distances and genetic distances between adjacent lines), setting negative rates to zero. But I think that what you did makes more sense. I did something similar to what you're doing in my paper (https://doi.org/10.1126/sciadv.aaw9206 ; Table S2) where I inferred maps on both GRCh37 and GRCh38, and then lifted over the maps from GRCh37 to GRCh38 and computed correlations between the map inferred on GRCh38 and the map inferred on GRCh37 but lifted over to GRCh38. The way I lifted over the map was a bit more naive than what you're doing -- I just converted the recombination rates to bed files and then ran LiftOver on the bed files without post-processing. I only computed correlations on positions that were included in an interval in both maps, and if positions appeared in more than one interval, I used the recombination rate from the last interval. In any case, the correlations were actually quite good. It will obviously depend on the method to infer the recombination maps, but I got correlations (spearman and pearson on the log-recombination rates) of ~0.98 and higher even at the single base pair resolution. Pearson on the natural scale varied much more depending on the population. I'm guessing this is because hotspots were inferred to have somewhat different heats depending on the build, driving the differences in correlation. But overall, my take was that running LiftOver had less of an effect than I might have thought (although of course YMMV especially for lifting over between genome builds that are less close than GRCh37 and GRCh38; also my method also uses unphased data, and I suspect that methods that use phased data might change more between different builds). Hope this helps, and overall it looks really great! |
|
@jeffspence thank you for taking a look at this!
Great catch, I hadn't considered the issue of biased means. I also just realized that I
I'm not either. By rearrangements I was thinking he might have meant only where contiguous intervals are reordered in the lifted-over assembly, but I'm really not sure. This also doesn't address how they dealt with gaps in the new assembly when they presumably arose.
This is comforting to know! |
|
I edited the code to use a properly weighted mean rate for the chromosome, per @jeffspence's suggestion. I also added the flag |
|
As per our conference call here's some code for seeing which target regions are covered in a chain file |
|
Thanks, @ndukler. I'll have a look. |
|
I found some issues with the code that I will be fixing over the coming days so we should hold off on merging for the time being |
|
It's annoying we're failing on circle because we can't import the new script - I think this is because we're asking nose to track coverage on the maintenance package. Might be simplest to just put @grahamgower, any thoughts on the simplest way out here? I don't think we want to try to write tests to cover this code, so I'm not sure what's the best thing to do here. |
|
Hmm... interesting. Does this go away if we stop asking for coverage in the maintenance folder? If its as simple as that, then I'd say lets just not bother tracking coverage there. |
Yes, that's probably the best thing all right. |
|
I've again updated this script. The newest version avoids doing a weighted average for all non-overlapping windows across the chromosome, and instead only performs weighted averaging for overlapping intervals in the lifted map (a relatively rare occurrence). This dramatically reduces the error when mapping back to the original assembly, and I think this is really the way to go. Additionally, I added a new option to fill gaps with either the chromosome mean, or with the mean of the flanking intervals which seems to be a better choice in most instances. I've also added maintenance code for the liftover of the comeron map from dm3 to dm6, as discussed in #592. |
|
Thanks @jradrion, this is now at the top of my queue of stdpopsim things. It's quite long though, so I probably wont get to it for a few more days. (And Jerome is on parental leave). Perhaps you could look into avoiding the circleci failure in the mean time? It might be sufficient to remove the |
|
@grahamgower OK, looks like removing |
|
@jradrion do we really want to get rid of those tests? |
|
This change made here doesn't change which tests are run. What it does do, is change which folders code coverage is reported for when running the tests (it was looking under both |
|
Hey @jradrion, I've looked over this code a couple of times now, but haven't tried running it. I think there are a few code formatting issues that should be fixed. Can you try running It would be nice to include a unittest or two for this in the test suite as well. How about including a small chain file for a small section of one chromosome, and store the pre-lifted and post-lifted rate maps for that? Then the unit test would check that the lift over script produces same output as the pre-calculated post-lifted maps. |
|
Thanks for looking it over, @grahamgower. I'll fix the formatting, add some tests, and update. |
There was a problem hiding this comment.
Looks great, thanks @jradrion!
There's a few high-level structural issues here that are resulting in us having a lot more code than we really need, but I think they can be resolved pretty easily. I'm happy to provide more input if the comments aren't clear.
maintenance/liftOver_assemblies.py
Outdated
| from matplotlib import gridspec | ||
|
|
||
|
|
||
| def progress_bar(percent, barLen = 50): |
There was a problem hiding this comment.
tqdm is a handy way of doing this.
maintenance/liftOver_assemblies.py
Outdated
| sys.stdout.flush() | ||
|
|
||
|
|
||
| def assign_task(tmpFile, task_q, nProcs): |
There was a problem hiding this comment.
Seems pretty heavyweight to need our own task queue here - have you tried using concurrent.futures?
maintenance/liftOver_assemblies.py
Outdated
| run all functions to translate and liftOver file types | ||
| """ | ||
| # multithread remapping | ||
| task_q = mp.JoinableQueue() |
There was a problem hiding this comment.
This does seem unneccesarily low-level here - there's a good example in the msprime code of using concurrent.futures along with a tqdm progress bar.
| from matplotlib import gridspec | ||
|
|
||
|
|
||
| def progress_bar(percent, barLen = 50): |
There was a problem hiding this comment.
This looks like a near-identical copy of the liftover_assemblies.py file. Having multiple copies of the same code is almost never a good idea, and will be a significant maintenance burden over time (especially if we have lots of species - consider what happens if we find a small bug in the liftover code, which has been copied n times).
How about we have a file liftover.py which provides the basic utilities that you're using here like hapmap_to_bed etc, and then the code specific to each liftover that we do is in its own python file that imports liftover?
There was a problem hiding this comment.
That is, basically just the main functions which you currently have?
|
@jeromekelleher Thank you for your comments! I'll look into implementing both tqdm and concurrent.futures, and I'll fix the redundancies. |
|
📖 Docs for this PR can be previewed here |
|
@jeromekelleher @grahamgower I had quite a difficult time trying to wrap my head around how exactly I would modify this code to use |
|
Also, is there a consensus for whether we want to have tests for all maintenance code? This code is still failing on CircleCi. Should I be writing tests for these scripts next? |
|
thank you @mufernando for helping me out of git hell. |
|
This is a big improvement, thanks @jradrion! I'm happy for this to be merged as-is. There's still a bit of cleanup that could be done (variable naming conventions are a bit inconsistent with a mixture of camelCase, CamelCase, ALLCAPS, and snake_case) but it's not that important for this maintenance code like this. The motivation for using concurrent.futures would be to remove redundant code, making it easier to understand what the file is actually for. Readers looking at the code might currently think there's some deep reason for needing to do tricky low-level concurrency stuff, and it takes a few minutes to figure out that there isn't. Anyway, this is nit-picking; awesome work, thanks! |
|
Thanks again @jradrion. And sorry for mess of CI changes you had to deal with during the life of this PR. Would you be able to open a couple of follow-up issues about the stuff raised by @jeromekelleher? |
|
Awesome, thanks @grahamgower. I will open issues about the comments made by @jeromekelleher. |
|
Thank you to @jeromekelleher, @grahamgower, @jeffspence, and @mufernando for all of your help with this. |
Here is my very unpolished code for doing liftOver between assemblies. I've tested this on humans and flies so far, and it is running without issue for me. The code requires that you have the UCSC liftOver tool installed and that you point to the appropriate chain file using the argument
--chainFile. The--winLenoption controls the interval length over which a weighted average recombination rate is calculated and--gapThreshcontrol the length of gaps where recombination rates are set to zero rather than using the chromosome average rate.An example command for lifting over the human map would be as follows: