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.

Sign up for GitHub

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

Jump to bottom

Updating workflow files to work with Igenome #388

Merged
merged 6 commits into from
Aug 28, 2024
Merged

Updating workflow files to work with Igenome #388

merged 6 commits into from
Aug 28, 2024

Conversation

nschcolnicov
Copy link
Contributor

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 you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/smrnaseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nf-test test main.nf.test -profile test,docker).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 26, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 7d534cd

+| ✅ 214 tests passed       |+
#| ❔   1 tests were ignored |#
!| ❗   3 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

❔ Tests ignored:

  • nextflow_config - Config default ignored: params.fastp_known_mirna_adapters

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-08-28 16:02:21

@nschcolnicov nschcolnicov changed the title Initialized changes Updating workflow files to work with Igenome Aug 27, 2024
@nschcolnicov nschcolnicov marked this pull request as ready for review August 27, 2024 13:42
atrigila
atrigila reviewed Aug 27, 2024
View reviewed changes
Copy link
Contributor

@atrigila atrigila left a comment

Choose a reason for hiding this comment

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

I added some comments to the code that can be addressed in this same PR or in other, if you think that is better.

The test case for this PR could be this issue: #359 which I think your solution addressed. We can add a simple nf-test that checks the presence of mirdeep2 files when using genome as input.

main.nf
Comment on lines +64 to +66
params.fasta,
params.mirtrace_species,
params.bowtie_index,
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I thought these had to be defined in the PIPELINE_INITIALISATION and the outputs from PIPELINE_INITIALISATION have to be passed to the main workflow. See:
https://github.com/nf-core/taxprofiler/blob/5d3ee5513a84f92773c8376c55b5f4da39835307/main.nf#L74-L91
or
https://github.com/nf-core/phaseimpute/blob/f2823b024800155cd87d70f406794324c4123dfd/main.nf#L140-L151
but I see sarek and rnaseq have different approaches as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave it as it is, as we discussed, since there doesn't seem to be a standard for how to set up the new template, we can always change it later if we have a reason to do so

workflows/smrnaseq.nf Outdated
ch_reads_for_mirna = FASTQ_FASTQC_UMITOOLS_FASTP.out.reads

// even if bowtie index is specified, there still needs to be a fasta.
// without fasta, no genome analysis.
if(params.fasta) {
if(fasta) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps since we are changing this for all fasta, we could adhere to adding the ch_ prefix:

"2.5 Input channel name structure
Input channel names SHOULD signify the input object type. For example, a single value input channel will be prefixed with val_, whereas input channels with multiple elements (e.g. meta map + file) should be prefixed with ch_."

https://nf-co.re/docs/guidelines/components/subworkflows

If this breaks things or you think it is too complicated we can open a new issue and address it separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally tackled this issue by converting value variables to value channels, and converting file string variables to path channels, but it created a lot of issues downstream. I want to tackle the conversion of all of these variables into channels in #390. To make sure that these are the only changes implemented in the branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this fasta variable is just equal to params.fasta, so I will convert it to val_fasta

workflows/smrnaseq.nf Outdated
@@ -181,8 +184,8 @@ workflow NFCORE_SMRNASEQ {
//
// SUBWORKFLOW: MIRTRACE
//
if (params.mirtrace_species) {
MIRTRACE(ch_mirtrace_inputs)
if (mirtrace_species) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as before, perhaps adding val_ before this channel name.

https://nf-co.re/docs/guidelines/components/subworkflows

@apeltzer
Copy link
Member

I think we can tackle the upstream points in two stages - lets update here already to match what @atrigila mentioned in terms of recommendations (ch_ and val_) for the points we need for this PR and then have a subsequent PR where we make sure to update it everywhere else necessary to match nf-core guidelines/recommendations more closely 👍🏻 Might come in handy too when we switch the local modules to nf-core/modules

apeltzer
apeltzer approved these changes Aug 27, 2024
View reviewed changes
Copy link
Member

@apeltzer apeltzer left a comment

Choose a reason for hiding this comment

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

Ok with the changes, would also go for changing the ch_ and val_ prefixes where necessary.

@nschcolnicov nschcolnicov force-pushed the fix_igenome branch 2 times, most recently from eb05af3 to aff93c1 Compare August 28, 2024 15:52
@nschcolnicov nschcolnicov merged commit 228212c into nf-core:dev Aug 28, 2024
16 checks passed
@nschcolnicov
Copy link
Contributor Author

I added some comments to the code that can be addressed in this same PR or in other, if you think that is better.

The test case for this PR could be this issue: #359 which I think your solution addressed. We can add a simple nf-test that checks the presence of mirdeep2 files when using genome as input.

Ill look at this separately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atrigila atrigila atrigila left review comments

@apeltzer apeltzer apeltzer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nschcolnicov @apeltzer @atrigila