Skip to content

Porting pipeline to DSL2 #43

Merged
SusiJo merged 44 commits intonf-core:dsl2from
SusiJo:dls2_new
Mar 29, 2023
Merged

Porting pipeline to DSL2 #43
SusiJo merged 44 commits intonf-core:dsl2from
SusiJo:dls2_new

Conversation

@SusiJo
Copy link
Collaborator

@SusiJo SusiJo commented Oct 21, 2022

Many thanks to contributing to qbic-pipelines/bamtofastq!

Please fill in the appropriate checklist below (delete whatever is not relevant). These are the most common things requested on pull requests (PRs).

PR checklist

  • 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, add test data to testdata/
  • 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
  • README.md is updated

Learn more about contributing: https://github.com/qbic-pipelines/bamtofastq/tree/master/.github/CONTRIBUTING.md

@SusiJo
Copy link
Collaborator Author

SusiJo commented Oct 21, 2022

Hi @FriederikeHanssen,
This is still a Draft PR so no need for a review yet. Since I had some branch issues on the other PR, I decided to make a fresh and cleaner PR. Also we decided to try integrating the pipeline into nf-core, therefore the labels have changed 😉

@FriederikeHanssen FriederikeHanssen mentioned this pull request Nov 2, 2022
8 tasks
SusiJo and others added 11 commits November 2, 2022 16:08
Co-authored-by: FriederikeHanssen <FriederikeHanssen@users.noreply.github.com>
Co-authored-by: FriederikeHanssen <FriederikeHanssen@users.noreply.github.com>
Co-authored-by: Gisela Gabernet <gisela.gabernet@qbic.uni-tuebingen.de>
Co-authored-by: FriederikeHanssen <FriederikeHanssen@users.noreply.github.com>
@FriederikeHanssen
Copy link
Contributor

it would probably be good if we start merging PRs soon-ish to the dsl2, so we can fix things independently. Otherwise, I will have to push to your branch which will cause chaos :D

@SusiJo
Copy link
Collaborator Author

SusiJo commented Mar 20, 2023

Linting is passing locally with nf-core tools 2.8.dev0. Might be failing because of a missing == in the pipeline template in the master branch that was added on the dev already. See here.

@SusiJo SusiJo marked this pull request as ready for review March 20, 2023 09:29
@SusiJo SusiJo merged commit 73c5542 into nf-core:dsl2 Mar 29, 2023
Copy link
Contributor

@matrulda matrulda left a comment

Choose a reason for hiding this comment

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

Great job! I've left a couple of comments.


- [ ] 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 you've added a new tool - have you followed the pipeline conventions in the [contribution docs](https://github.com/nf-core/bamtofastq/tree/master/.github/CONTRIBUTING.md)- [ ] If necessary, also make a PR on the nf-core/bamtofastq _branch_ on the [nf-core/test-datasets](https://github.com/nf-core/test-datasets) repository.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [ ] If you've added a new tool - have you followed the pipeline conventions in the [contribution docs](https://github.com/nf-core/bamtofastq/tree/master/.github/CONTRIBUTING.md)- [ ] If necessary, also make a PR on the nf-core/bamtofastq _branch_ on the [nf-core/test-datasets](https://github.com/nf-core/test-datasets) repository.
- [ ] If you've added a new tool - have you followed the pipeline conventions in the [contribution docs](https://github.com/nf-core/bamtofastq/tree/master/.github/CONTRIBUTING.md)
- [ ] If necessary, also make a PR on the nf-core/bamtofastq _branch_ on the [nf-core/test-datasets](https://github.com/nf-core/test-datasets) repository.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's actually a typo in the pipeline template :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this file be removed? It looks like multiqc_config.yml is the one that is used. In that case, should perhaps the top modules info should be moved to that config?

| `sample` | Custom sample name. |
| `mapped` | Full path to input BAM/CRAM file. File has to have the extension ".bam" or ".cram". |
| `index` | If available provide full path to input BAI/CRAI index file. File has to have the extension ".bam.bai" or ".cram.crai". |
| `filetype` | For input BAM files the filetype hast to be "bam" and for input CRAM files, the filetype needs to be "cram". |
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the description can simply be something like: "Type of input file. Options: bam or cram"

Unfortunately, at the time of writing, FastQC [doesn't support](https://github.com/s-andrews/FastQC/issues/54) CRAM files as input. Hence, a benefit of converting CRAM files to BAM format as opposed to converting directly to FASTQ format is that you can perform QC before the final conversion.
### `--fasta`

Use this option to indicate which reference genome FASTA file to use when decompressing CRAM files. It will only work if the reference genome FASTA file listed in the CRAM header is available (_e.g._ via HTTP/FTP or on the local file system). Otherwise, you will need to use the [`--fasta`](#--fasta) option. You can check which reference FASTA file is indicated in the CRAM header with the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this section is a bit confusing. If I understand it right, this parameter is only needed if an accessible fasta file isn't specified the header?

Suggestion:
When converting a CRAM file the fasta file specified in the CRAM header is used to decompress the file. If that file isn't available, you will need to specify an alternative path using the --fasta option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Sounds much better!

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
*/

params.chr = WorkflowMain.getGenomeAttribute(params, 'chr')
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? Afaik this function is used to extract info from a genomes config (like igenomes.config). Don't feel like it's applicable here? I could be missing something though :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good spot, thanks!

include { SAMTOOLS_VIEW as SAMTOOLS_CHR } from '../modules/nf-core/samtools/view/main'
include { SAMTOOLS_VIEW as SAMTOOLS_PE } from '../modules/nf-core/samtools/view/main'
include { SAMTOOLS_INDEX as SAMTOOLS_CHR_INDEX } from '../modules/nf-core/samtools/index/main'
include { CHECK_IF_PAIRED_END } from '../modules/local/check_paired_end'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be imported in the above section?

SAMTOOLS_CHR.out.cram)
SAMTOOLS_CHR_INDEX(samtools_chr_out)
ch_input = samtools_chr_out.join(Channel.empty().mix(SAMTOOLS_CHR_INDEX.out.bai,
SAMTOOLS_CHR_INDEX.out.crai))
Copy link
Contributor

Choose a reason for hiding this comment

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

Align parameters?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do

// Index File if not PROVIDED -> this also requires updates to samtools view possibly URGH

// MAP - MAP
SAMTOOLS_VIEW_MAP_MAP(input, fasta, [])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure I'm just missing something obvious, but I don't get how the samtools flags are passed to the module. I guess they are part of the meta data somehow, but I can't seem to find where.

Copy link
Collaborator Author

@SusiJo SusiJo Apr 5, 2023

Choose a reason for hiding this comment

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

They are specified in the modules.config for each named process.

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.

3 participants