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

Adjustments for #76 , rename certain options to be more explicit #81

Merged
merged 16 commits into from
Nov 19, 2018

Conversation

apeltzer
Copy link
Member

@apeltzer apeltzer commented Nov 5, 2018

This renames a couple of things to different names, to make sure people understand what these options mean.

cf #76

Namely:

bam_keep_mapped_only ==> bam_analyse_mapped_only

bam_filter_reads ==> bam_retain_all_reads

  • This comment contains a description of changes (with reason)
  • If you've fixed a bug or added code that should be tested, add tests!
  • If necessary, also make a PR on the nf-core/eager branch on the nf-core/test-datasets repo
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • Make sure your code lints (nf-core lint .).
  • Documentation in docs is updated
  • CHANGELOG.md is updated

Learn more about contributing: https://github.com/nf-core/eager/tree/master/.github/CONTRIBUTING.md

docs/usage.md Outdated

This can be used to only keep mapped reads for downstream analysis. By default turned off, all reads are kept in the BAM file. Unmapped reads are stored both in BAM and FastQ format e.g. for different downstream processing.
This can be used to only keep mapped reads in the BAM file for downstream analysis. By default turned off, all reads are kept in the BAM file. Unmapped reads are stored both in BAM and FastQ format e.g. for different downstream processing.
Copy link
Member

@jfy133 jfy133 Nov 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe change following line to: 'By default turned off, where all reads are kept in the bam file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@jfy133
Copy link
Member

jfy133 commented Nov 5, 2018

Generally looks good, made a minor suggestion for description.

only think that isn't clear to me now is what if you want to discard unmapped reads entirely?

Unless I misunderstand - I currently see only two cases: 1) where mapped and unmapped reads are retained but in separate BAMs and 2) where mapped and unmapped reads are in the same BAM.

I am thinking for cases where people have storage space issues and want to minimise their data to stuff of interest. (This would be another issue, unless I have misunderstood)

@apeltzer
Copy link
Member Author

apeltzer commented Nov 6, 2018

I added an option that can remove the unrequired unmapped reads on user request. By default, its turned off, but of course can be turned on by users if they want to :-)

Also added documentation on that @jfy133 let me know what you think .-)

docs/usage.md Outdated

This can be used to only keep mapped reads for downstream analysis. By default turned off, all reads are kept in the BAM file. Unmapped reads are stored both in BAM and FastQ format e.g. for different downstream processing.
This can be used to only keep mapped reads in the BAM file for downstream analysis. By default turned off, where all reads are kept in the bam file. Unmapped reads are stored both in BAM and FastQ format e.g. for different downstream processing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly rephrase again for clarity (my bad):
Use only mapped reads in the BAM file for downstream analysis. Unmapped reads are stored in a separate BAM and FASTQ format e.g. for different downstream processing. By default turned off, where default behaviour is to keep both mapped and unmapped reads in the output BAM file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it makes sense to instead have a independent flag to save unmapped reads in FASTQ rather than BAM format. In this case we will be make redundancy.

docs/usage.md Outdated

### `--bam_discard_unmapped_entirely`

This discards all unmapped and extracted reads entirely. By default, this is turned off.

### `--bam_keep_all`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference of this flag verses --bam_retain_all_reads?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--bam_discard_unmapped_entirely

Removes the unmapped reads file only, the BAM file contains only mapped reads and unmapped reads are entirely discarded (no fastq/bam at all present).

I guess I'll have to give this another proper thought. Right now bam_retain_all_reads just is the thing to turn on BAM filtering in general.

@apeltzer apeltzer merged commit d91226f into dev Nov 19, 2018
@apeltzer apeltzer deleted the rename_bam_flags branch November 19, 2018 14:45
@jfy133 jfy133 mentioned this pull request Nov 20, 2018
@apeltzer apeltzer mentioned this pull request Dec 9, 2018
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.

2 participants