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

Important! Template update for nf-core/tools v2.13.1 #599

Merged
merged 25 commits into from
May 6, 2024
Merged

Important! Template update for nf-core/tools v2.13.1 #599

merged 25 commits into from
May 6, 2024

Conversation

nf-core-bot
Copy link
Member

Version 2.13.1 of nf-core/tools has just been released with updates to the nf-core template. This automated pull-request attempts to apply the relevant updates to this pipeline.

Please make sure to merge this pull-request as soon as possible, resolving any merge conflicts in the nf-core-template-merge-2.13.1 branch (or your own fork, if you prefer). Once complete, make a new minor release of your pipeline.

For instructions on how to merge this PR, please see https://nf-co.re/docs/contributing/sync/.

For more information about this release of nf-core/tools, please see the v2.13.1 release page.

@jfy133
Copy link
Member

jfy133 commented Mar 26, 2024 •
edited
Loading

OK I've started this now, things that I will need to re-add back in once I've done the template merge bit:

TODO

  • Move contents of INPUT_CHECK to new subworkflow, check that output channels (raw_short, raw_long etc.) are prepared correctly
    • All --input validation code
    • Also include // Ensure run IDs are unique within samples (also prevents duplicated sample names)
    • All --assembly_input validation code
  • Move all input validation in lib/ to the new subworkflow
    • Update citation string at citation() function (can't remmeber wher enow) to include official mag one -> Already added in template!
    • Move validation from WorkflowMag.nf to validateInputPArametes
    • Update email sending to include busco_failed_bins(?) -> OPTED AGAINST THIS!
  • Re-add the citation text that in mag.nf get's appended to the end of the paramsSummaryLog()
  • Check input file existence still working (likely will need nextflow_schema.json update)
  • Make schema input for assembly input

Important

Must document/announce no more direct input!

Tests

General

  • Normal test ✅
  • Without run column ✅
  • Without group column ✅
  • Single end run ✅
  • long read (hybrid) test ✅

Error tests

Standard input

  • ✅ sr1 exists
  • ✅ sr2 exists
  • ✅ lr exists -> I guess it doesn't need to, just automatically runs short read only (no hybrid)(
  • run column exists and is not empty (i.e., can't be empty string)
    • ✅ get error if duplicate sample_id but no run id
  • ✅ sr1 is required column
  • ✅ if sr2, sr1 also required
  • ✅ if lr, sr1 and sr2 also required
  • ✅ if no sr2, --single_end is required -> tried to run with an empty R1
  • ✅ if sr2, --single_end MUST NOT be specified

Test input files:
template-merge-input-tests.zip

Test assembly input files:

template-merge-tests-assemblyinput.zip

Assembly input:

  • ✅ --assembly_input can only be used if --input supplied
  • ✅ FASTA column must not be empty
  • ✅ Assembly must not be empty (presumably one of supported)
  • ✅ IDs between --assembly_input and --input must match
    • ✅ also during coassembly

Other issues:

  • ✅ in validateInputParameters how to deal with the odd hybrid variable -> [FIXED] do the same as in current version (i.e., update the hybird variable that is defined in the main script, from wtihin a map{})

@github-actions GitHub Actions
Copy link

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

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 41e8956

+| ✅ 212 tests passed       |+
#| ❔   2 tests were ignored |#
!| ❗   5 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in main.nf: Remove this line if you don't need a FASTA file [TODO: try and test using for --host_fasta and --host_genome]
  • 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!
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.13.1
  • Run at 2024-05-06 09:00:43

@jfy133
Copy link
Member

jfy133 commented Apr 19, 2024 •
edited
Loading

  1. I'm opting against adding the busco_failed_bins to the email, users would discover this by looking at the results, and now we have checkM etc. which we dont' get the report it seems unnecessarily specific.

It will reduce the load during future template merges

  1. Removing the custom check that --input must be suppilied if --assembly_input supplied, as --input is always required

jfy133
jfy133 reviewed Apr 19, 2024
View reviewed changes
nextflow_schema.json Outdated Show resolved Hide resolved
jfy133
jfy133 reviewed Apr 19, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
jfy133
jfy133 reviewed Apr 19, 2024
View reviewed changes
nextflow_schema.json Outdated Show resolved Hide resolved
maxulysse
maxulysse reviewed Apr 30, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
maxulysse
maxulysse approved these changes Apr 30, 2024
View reviewed changes
Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

minor comments, looking good

@CarsonJM
Copy link
Contributor

Just went through the code/ran some tests and this update looks great! Would probably be worth adding a test_assembly_input profile, but that can probably be added when we implement nf-test

jfy133 and others added 4 commits May 6, 2024 09:57
@jfy133
Copy link
Member

jfy133 commented May 6, 2024

The failure on the test_binrefinement test is apparently a gzipped FASTA file is getting through to cONCOCT cut-up-contigs and the python tool can't read GZIPPEd files

@jfy133
Copy link
Member

jfy133 commented May 6, 2024

Fixed, finallY! 🤞 all good!

@jfy133 jfy133 merged commit 8392af9 into dev May 6, 2024
15 checks passed
@jfy133 jfy133 deleted the nf-core-template-merge-2.13.1 branch May 6, 2024 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfy133 jfy133 jfy133 left review comments

@maxulysse maxulysse maxulysse approved these changes

@CarsonJM CarsonJM CarsonJM 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.
4 participants
@nf-core-bot @jfy133 @CarsonJM @maxulysse