Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid passing too many parameters. #79

Merged
merged 3 commits into from
Mar 21, 2022
Merged

Avoid passing too many parameters. #79

merged 3 commits into from
Mar 21, 2022

Conversation

haowenz
Copy link
Owner

@haowenz haowenz commented Feb 25, 2022

NOTE: this PR would change MAPQ for single-end and Hi-C mappings.

This code refactor helped identify and fix bugs for single-end read and Hi-C read MAPQ. It is clear that the parameters were passed in the wrong order, which led to wrong MAPQ for single-end reads at the following line.

kNegative, num_negative_candidates, repetitive_seed_length, mapq,

For Hi-C reads, the bug is at the following line.

negative_mappings1, negative_split_sites1, num_positive_candidates2,

Here num_positive_candidates2 should be num_negative_candidates2.

After fixing this, the single-end simulated read MAPQ seems to get improved as shown in the following figure.
roc-color_single

Tested on all benchmark datasets. No change on paired-end ChIP-seq or scATAC-seq mappings was observed. Hi-C mappings only slightly changed. And the MAPQ of many single-end ChIP-seq mappings changed.

Reducing the parameters for verification functions seems to cause the change of single-end mappings and a few good single-end mappings were missed. But when running Chromap on a single missed read, the mapping of it was reported. Thus, this might be a problem cause by cache. So this PR will be examined again after @mourisl fix the cache.

@haowenz
Copy link
Owner Author

haowenz commented Mar 21, 2022

Checked again after the cache was fixed. The single-end mappings look okay. So merge this PR.

@haowenz haowenz marked this pull request as ready for review March 21, 2022 18:47
@haowenz haowenz merged commit 67d6781 into master Mar 21, 2022
@haowenz haowenz deleted the refactor branch March 21, 2022 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant