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

Update modules #512

Merged
merged 32 commits into from
Sep 9, 2024
Merged

Update modules #512

merged 32 commits into from
Sep 9, 2024

Conversation

LilyAnderssonLee
Copy link
Contributor

Close #403

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/taxprofiler branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • 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 2, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit df25a1b

+| ✅ 268 tests passed       |+
!| ❗   1 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in ci.yml: You can customise CI pipeline run tests as required

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-09-07 20:35:45

@jfy133
Copy link
Member

jfy133 commented Aug 8, 2024

To summarise the issue:

  • DIAMOND doesn't actually accept paired end input (Silent fail in taxprofiler <= 1.1.8, it would just take the first file)
  • A new check in the module expects a single file not a list, but we sometimes send pairs, and so get.Extesion() doesn't work if there is >= 1 file
  • With nopreprocessing there is no read-merging, so remains as pairs rather than a single file

@LilyAnderssonLee and my current thinking:

  • Have a warning that if paried-end data is input + diamond turned on, give a warning it will be filtered out
  • Filter out paire-dend data before it goes into diamond

@LilyAnderssonLee
Copy link
Contributor Author

LilyAnderssonLee commented Aug 28, 2024 •
edited
Loading

ganon_log.zip
@jfy133 The multiqc module has been updated to the latest version, but ganon still can't be included in the MultiQC report because the log file generated by the latest ganon version 2.0.0 differs from the one generated by version 1.5.1 used in the MultiQC ganon module.

@LilyAnderssonLee
Copy link
Contributor Author

LilyAnderssonLee commented Aug 30, 2024 •
edited
Loading

nanoq has been added to the MultiQC report.
multiqc_report.html.zip

@LilyAnderssonLee
Copy link
Contributor Author

All modules have been updated except for motus/profile. I have opened a PR to update this module. But there are some issues running nf-test at the global setup MOTUS_DOWNLOADDB due to the large database. I believe we need to prepare a smaller test database for motus, which will take some time. Since Sofia is updating motus/profile for long reads, I’m considering if we could skip the update for motus/profile for now and include it in the third release. This way, we could proceed with the release this week. What do you think @nf-core/taxprofiler ?

@jfy133
Copy link
Member

jfy133 commented Sep 4, 2024 •
edited
Loading

I had one last idea for motus/profile, so testing that otherwise yes we can skip that one for now

Update: appears to be working now, just an MD5 checksum difference for conda for some reason

@LilyAnderssonLee
Merge pull request nf-core#518 from nf-core/fix-nanopore-fasta
5e0d556 
Fix nanopore fasta input being incorrectly channeled
sofstam
sofstam reviewed Sep 5, 2024
View reviewed changes
Copy link
Collaborator

@sofstam sofstam left a comment

Choose a reason for hiding this comment

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

Shall it be documented in the usage that Nanoq is going to be the default option if a user chooses filtering?

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
subworkflows/local/longread_hostremoval.nf Show resolved Hide resolved
sofstam
sofstam reviewed Sep 5, 2024
View reviewed changes
docs/usage.md Show resolved Hide resolved
sofstam
sofstam reviewed Sep 6, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
assets/schema_input.json Show resolved Hide resolved
@jfy133
Copy link
Member

jfy133 commented Sep 6, 2024

@LilyAnderssonLee I think we want unique file names for ALL types of files (fastq/fasta) etc!

@jfy133
Copy link
Member

jfy133 commented Sep 6, 2024

@nf-core-bot fix linting

@LilyAnderssonLee
Copy link
Contributor Author

LilyAnderssonLee commented Sep 7, 2024 •
edited
Loading

@LilyAnderssonLee I think we want unique file names for ALL types of files (fastq/fasta) etc!

Initially, I thought it wasn't necessary since, for FASTQ files, the reads channel from preprocessing or host removal is defined as ${meta.id}_${meta.run_accession} which is unique. But if someone wants to run profiling directly that could be a problem. I suppose I could add unique:true to the FASTQ files.

@jfy133
Copy link
Member

jfy133 commented Sep 8, 2024

Initially, I thought it wasn't necessary since, for FASTQ files, the reads channel from preprocessing or host removal is defined as ${meta.id}_${meta.run_accession} which is unique. But if someone wants to run profiling directly that could be a problem. I suppose I could add unique:true to the FASTQ files.

You're right it's not so bad after the first couple of steps as indeed we come up with our own names.

But for the initial steps it might be safer, and also (now I think about it) , it would help prevent any copy pasting errors when creating the samplesheet too so I think it would be worth it :).

If someone has the exact same file name for different samples or libraries on their system, this is dangerous for them too so we can nudge them to also be more careful with that 😬

@sofstam sofstam self-requested a review September 9, 2024 08:18
@LilyAnderssonLee LilyAnderssonLee merged commit c6dfb84 into nf-core:bouncy-basenji Sep 9, 2024
24 checks passed
@LilyAnderssonLee LilyAnderssonLee mentioned this pull request Sep 11, 2024
Closed
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

@sofstam sofstam sofstam 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
@LilyAnderssonLee @jfy133 @sofstam