rnaseq icon indicating copy to clipboard operation
rnaseq copied to clipboard
verified publisher icon nf-core

Improved ext.args consolidation for STAR and TRIMGALORE

Open MatthiasZepper opened this issue 1 year ago • 3 comments

Occasionally, people were surprised that parameters provided to extra_star_align_args could still conflict with the default configuration (e.g. #1046).

As a convenience parameter, I felt that it should never take precedence over the actual module configuration, because it seemed more error-prone to silently overwrite the module config with something that (as the parameter name suggests) is meant to be extra. Hence, I preferred that a custom module config should be used.

However, I said that I would be open to reconsider and since now also @tdanhorn and @CharlotteAnne suggested that change, I gave in. This PR now improves the extra_args parameter consolidation so it can override presets from the config.

PR checklist

  • [x] This comment contains a description of changes (with reason).
  • [ ] If you've fixed a bug or added code that should be tested, add tests!
  • [x] Make sure your code lints (nf-core lint).
  • [x] 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>).
  • [x] CHANGELOG.md is updated.

MatthiasZepper avatar Mar 08 '24 20:03 MatthiasZepper

nf-core lint overall result: Passed :white_check_mark: :warning:

Posted for pipeline commit 6ab2e44

+| ✅ 167 tests passed       |+
#| ❔   9 tests were ignored |#
!| ❗   7 tests had warnings |!

:heavy_exclamation_mark: Test warnings:

  • files_exist - File not found: assets/multiqc_config.yml
  • files_exist - File not found: .github/workflows/awstest.yml
  • files_exist - File not found: .github/workflows/awsfulltest.yml
  • 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
  • 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!

:grey_question: Tests ignored:

  • files_exist - File is ignored: conf/modules.config
  • nextflow_config - Config default ignored: params.ribo_database_manifest
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: assets/email_template.html
  • files_unchanged - File ignored due to lint config: assets/email_template.txt
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore or pyproject.toml
  • actions_ci - actions_ci
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/rnaseq/rnaseq/.github/workflows/awstest.yml
  • multiqc_config - 'assets/multiqc_config.yml' not found

:white_check_mark: Tests passed:

Run details

  • nf-core/tools version 2.13.1
  • Run at 2024-04-17 10:26:52

github-actions[bot] avatar Mar 08 '24 20:03 github-actions[bot]

I did manually test the change with the test profile and providing extra_star_align_args = '--outFilterMultimapNmax 50', but it felt excessive to write a separate nf-test only for this one parameter and wasn't sure if I can muddle it through with an existing test without derailing those?

Screen Shot 2024-03-08 at 21 59 49

MatthiasZepper avatar Mar 08 '24 21:03 MatthiasZepper

Thank your for doing this, @MatthiasZepper. I think it follows the principle of "least surprise". Don't know how easy it would be, but if you wanted to avoid people inadvertently overwriting preset options, you could test for that and issue a warning. I don't consider it strictly necessary, though -- people who use this option should know what they are doing.

tdanhorn avatar Mar 10 '24 04:03 tdanhorn

Seems the tests are failing. Before I try to troubleshoot code in a language I do not know... do you, Jonathan, right away know where the problem is?

MatthiasZepper avatar Apr 16 '24 18:04 MatthiasZepper

Sorry, my bad. These should do it.

That looks much better! Thank you very much!

There are two tests failing, but to me, it doesn't seem that this is anyway related to this PR.

MatthiasZepper avatar Apr 17 '24 11:04 MatthiasZepper