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

Drop db_params patterns for bracken and kmcp #476

Merged
merged 3 commits into from
Apr 24, 2024
Merged

Drop db_params patterns for bracken and kmcp #476

merged 3 commits into from
Apr 24, 2024

Conversation

LilyAnderssonLee
Copy link
Contributor

Fix the issue #475

The original definitions of db_params of bracken and kmcp in `db_check.nf``

    if ( row.tool == 'bracken' && row.db_params && !row.db_params.contains(";") )  error("[nf-core/taxprofiler] ERROR: Invalid database db_params entry. Bracken requires a semi-colon if passing parameter. Error in: ${row}")
    if ( row.tool == 'kmcp' && row.db_params && row.db_params.matches(".*;\$"))  error ("[nf-core/taxprofiler] ERROR: Invalid database db_params entry. KMCP only requires a semi-colon if passing arguments to KMCP profile, in cases of which the arguments should go after the semi-colon. Error in: ${row}")

The database.csv used in this test:

tool,db_name,db_params,db_path
bracken,db1,;-r 150,https://github.com/nf-core/test-datasets/raw/taxprofiler/data/database/bracken/testdb-bracken.tar.gz
kraken2,db2,--quick,https://github.com/nf-core/test-datasets/raw/taxprofiler/data/database/kraken2/testdb-kraken2.tar.gz
kmcp,db1,;\$--min-kmers 30;\$--min-query-cov 0.5,https://github.com/nf-core/test-datasets/raw/taxprofiler/data/database/kmcp/test-db-kmcp.tar.gz

The Bracken output files in this test:
image

kmcp output files in this test:
image

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 (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 Apr 24, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit d6c5f75

+| ✅ 193 tests passed       |+
!| ❗   2 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in ci.yml: You can customise CI pipeline run tests as required
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

✅ Tests passed:

Run details

  • nf-core/tools version 2.13.1
  • Run at 2024-04-24 13:06:02

@jfy133
Copy link
Member

jfy133 commented Apr 24, 2024

I'm trying work out the best way to do this now:

Reading our docs:

image

And comparing with the code, it looks like to me actually we don't require the ; even if you're running bracken. We did in the past but we relaxed: you only need a semi-colon if you want to supply to both kraken/bracken or just kraken. If you want to send to just kraken, no semi-0colon needed.

So in this case the new pattern wouldn't work with that, as it's requiringa semi-colon

Hence... I think we just need to remove the pattenr entirely? What do you think?

@LilyAnderssonLee
Copy link
Contributor Author

It sounds wise to me to remove the db_parmas patters for bracken and kmcp.

@LilyAnderssonLee LilyAnderssonLee changed the title Correct db_params patterns of bracken and kmcp Drop db_params patterns for bracken and kmcp Apr 24, 2024
@jfy133 jfy133 merged commit e8ce046 into nf-core:dev Apr 24, 2024
24 checks passed
@jfy133
Copy link
Member

jfy133 commented Apr 24, 2024

Thank you @LilyAnderssonLee ! I think we can do a patch release already (if you don't have time I will do it tomorrow evening)

@jfy133
Copy link
Member

jfy133 commented Apr 24, 2024

Unless @sofstam would have time

@LilyAnderssonLee
Copy link
Contributor Author

@jfy133 I have some time tomorrow morning before 10:00. I hope I can manage a patch release before then.

@sofstam
Copy link
Collaborator

sofstam commented Apr 24, 2024 via email

@LilyAnderssonLee LilyAnderssonLee deleted the database_schema_bug branch June 25, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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