Skip to content

Remove --report_scree parameter#638

Open
delfiterradas wants to merge 3 commits intonf-core:devfrom
delfiterradas:report_scree
Open

Remove --report_scree parameter#638
delfiterradas wants to merge 3 commits intonf-core:devfrom
delfiterradas:report_scree

Conversation

@delfiterradas
Copy link

@delfiterradas delfiterradas commented Feb 13, 2026 •
edited
Loading

Closes #637

  • Remove --report_scree parameter and always include scree plot in report

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/differentialabundance 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).

@delfiterradas delfiterradas self-assigned this Feb 13, 2026
@github-actions

This comment was marked as outdated.

Sign in to view
@delfiterradas delfiterradas changed the base branch from master to dev February 13, 2026 15:23
@github-actions
Copy link

github-actions bot commented Feb 13, 2026 •
edited
Loading

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 24a5634

+| ✅ 381 tests passed       |+
#| ❔  10 tests were ignored |#
#| ❔   1 tests had warnings |#
!| ❗  20 tests had warnings |!
Details

❗ Test warnings:

  • readme - README contains the placeholder zenodo.XXXXXXX. This should be replaced with the zenodo doi (after the first release).
  • pipeline_todos - TODO string in nextflow.config: Update the field with the details of the contributors to your pipeline. New with Nextflow version 24.10.0
  • pipeline_todos - TODO string in base.config: Check the defaults for all processes
  • 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!
  • schema_lint - Parameter input is not defined in the correct subschema (input_output_options)
  • schema_description - No description provided in schema for parameter: deseq2_seed
  • schema_description - No description provided in schema for parameter: dream_p_value
  • schema_description - No description provided in schema for parameter: dream_lfc
  • schema_description - No description provided in schema for parameter: dream_confint
  • schema_description - No description provided in schema for parameter: dream_proportion
  • schema_description - No description provided in schema for parameter: dream_stdev_coef_lim
  • schema_description - No description provided in schema for parameter: dream_trend
  • schema_description - No description provided in schema for parameter: dream_robust
  • schema_description - No description provided in schema for parameter: dream_winsor_tail_p
  • schema_description - No description provided in schema for parameter: dream_ddf
  • schema_description - No description provided in schema for parameter: dream_reml
  • schema_description - No description provided in schema for parameter: dream_apply_voom
  • schema_description - No description provided in schema for parameter: dream_adjust_method

❔ Tests ignored:

  • files_exist - File is ignored: assets/multiqc_config.yml
  • nextflow_config - Config default ignored: params.report_file
  • nextflow_config - Config default ignored: params.logo_file
  • nextflow_config - Config default ignored: params.css_file
  • nextflow_config - Config default ignored: params.citations_file
  • files_unchanged - File ignored due to lint config: .github/CONTRIBUTING.md
  • files_unchanged - File ignored due to lint config: assets/nf-core-differentialabundance_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-differentialabundance_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-differentialabundance_logo_dark.png
  • multiqc_config - multiqc_config

❔ Tests fixed:

✅ Tests passed:

Run details

  • nf-core/tools version 3.5.1
  • Run at 2026-02-13 17:45:37

@delfiterradas delfiterradas marked this pull request as ready for review February 13, 2026 16:01
@pinin4fjords
Copy link
Member

pinin4fjords commented Feb 19, 2026

Can we talk about the param removals guys? We might have chatted previously, but remind me why we care about them being there?

@grst
Copy link
Member

grst commented Feb 20, 2026

My motivation is stated in #484 (comment)

This pipeline has an enormous number of parameters. Each of them creates a maintenance burden (keeping docs up-to-date, take params into account during modifications) and make it more complicated for users to understand what they need to specify in order to run the pipeline correctly.

And your response in #484 (comment)

We should address these one by one. e.g. the fold change logging is method dependent.

But I agree with the principle, I may have got a little param-happy. Folks can still supply these things via custom config and ext.args.

I understand that for some in the list there may be reasons to keep them, but I think it's a valid appraoch to open a PR with the suggestion to remove/modify them and then discuss it.

For this particular one, I don't see why there would be a flag for including the scree plot, but not for other sections. If one wants to customize the quarto report, it's already possible to modify the qmd file and pass it to the pipeline.

From my perspective that would be enough, but if we want the main report to be more configurable, then I think it would be preferable to have a param like

include_param_modules: ["volcano", "scree", "de_table", "pca", ... ]

instead of having one flag for each section.

@pinin4fjords
Copy link
Member

pinin4fjords commented Feb 23, 2026

OK, how about we stick with the principle that you can remove a param if users still have a way of doing the same thing? e.g. if I can use ext.args to accomplish the same objective that's fine (so that would allow a lot of the individual tool-wise params to be stripped, which is what I was imagining in that original discussion).

@grst
Copy link
Member

grst commented Feb 25, 2026

it's already possible to modify the qmd file and pass it to the pipeline.

does that count?

@pinin4fjords
Copy link
Member

pinin4fjords commented Feb 25, 2026

it's already possible to modify the qmd file and pass it to the pipeline.

does that count?

No, if that counted everything would be allowed!

@grst
Copy link
Member

grst commented Feb 26, 2026

What bothers me most about this parameter is the inconsistency. It's the only section of the quarto report that you can switch off, while there's no option for the others.

I don't think that it scales well to add a separate parameter for each section. So I'd propose this instead:

[...] I think it would be preferable to have a param like

include_param_modules: ["volcano", "scree", "de_table", "pca", ... ]

instead of having one flag for each section.

Another advantage is that this would still work with a custom quarto report that might have different "modules".

WDYT?

@pinin4fjords
Copy link
Member

pinin4fjords commented Feb 26, 2026

Yep, I'm up for that, since you're not taking away the control :-)

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

Reviewers

@grst grst Awaiting requested review from grst

@pinin4fjords pinin4fjords Awaiting requested review from pinin4fjords

At least 1 approving review is required to merge this pull request.

Assignees

@delfiterradas delfiterradas

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Include scree plot by default

3 participants

@delfiterradas @pinin4fjords @grst