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

Fix coverage summary #126

Merged
merged 5 commits into from
Feb 14, 2024
Merged

Fix coverage summary #126

merged 5 commits into from
Feb 14, 2024

Conversation

jmtsuji
Copy link
Collaborator

@jmtsuji jmtsuji commented Feb 14, 2024

Fixes a couple intertwined issues in rule summarize_contigs_by_coverage that caused errors when running rotary in long read only mode, to address one part of #118

In the previous version of rotary, long-read only mode actually worked with rule summarize_contigs_by_coverage, but only when a coverage filter threshold was supplied in the config file. The rule resulted in an error when no long read coverage filter thresholds were supplied (which is the config default) in long-read only mode.

In this update, the following changes were made to address this problem:

  • rule summarize_contigs_by_coverage was moved inside a conditional block (as it should have always been) so that it only runs when some kind of coverage filtration is requested
  • a basic report summarizing the results of coverage filtration was created -- this is symlinked to {sample}/polish/{sample}_contig_info.tsv. This report is made whether coverage filtration is performed or not.
  • the above report is used to parse out which contigs to run circularization on in checkpoint split_circular_and_linear_contigs -- before, no report was made if coverage filtration was not run, causing this checkpoint to fail.

Because of how rule summarize_contigs_by_coverage was moved into an existing conditional block in polish.smk, the git diff does not show the file changes very cleanly -- my apologies!

In the long term, I think it might make sense to move the code in summarize_contigs_by_coverage into a Python utility script -- this would make it easier to write/run tests on some of the logic being performed in this code.

Let me know if you have any thoughts -- thanks!

@jmtsuji jmtsuji added the bug Something isn't working label Feb 14, 2024
@LeeBergstrand
Copy link
Collaborator

LeeBergstrand commented Feb 14, 2024

In the long term, I think it might make sense to move the code in summarize_contigs_by_coverage into a Python utility script -- this would make it easier to write/run tests on some of the logic being performed in this code.

@jmtsuji This sounds good. Feel free to create an issue for this.

@jmtsuji
Copy link
Collaborator Author

jmtsuji commented Feb 14, 2024

In the long term, I think it might make sense to move the code in summarize_contigs_by_coverage into a Python utility script -- this would make it easier to write/run tests on some of the logic being performed in this code.

@jmtsuji This sounds good. Feel free to create an issue for this.

Will include in issue #120

@LeeBergstrand
Copy link
Collaborator

@jmtsuji Looks good to me at a glance. Feel free to merge in.

@jmtsuji jmtsuji merged commit 56aa146 into develop Feb 14, 2024
@jmtsuji jmtsuji deleted the fix_coverage_summary branch February 14, 2024 06:31
@jmtsuji jmtsuji mentioned this pull request Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants