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.

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

Added logic for transformation method #206

Merged
merged 19 commits into from
Jun 21, 2023
Merged

Conversation

WackerO
Copy link
Collaborator

@WackerO WackerO commented Jun 14, 2023

This PR adds a threshold for size factor variance (0.05) above which the pipeline will automatically use rlog transformation even if the user requests vst (can be overridden by user).
Additionally, heatmaps are now saved inside a trycatch (especially png() often throws an error), meanSdPlot has now correct plot titles depending on rlog/vst and < or > comparisons (is pval < 0.05) are now always <= or >= (is pval <= 0.05) as this is also what the gost() function does.

@qbicStefanC

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/rnadeseq 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>).
  • 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
Copy link

github-actions bot commented Jun 14, 2023

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 646ffa4

+| ✅ 125 tests passed       |+
#| ❔  27 tests were ignored |#
!| ❗  10 tests had warnings |!

❗ Test warnings:

  • readme - README did not have a Nextflow minimum version badge.
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your prefered methods description, e.g. add publication citation for this pipeline
  • pipeline_todos - TODO string in rnadeseq.nf: Add all file path parameters for the pipeline to the list below
  • pipeline_todos - TODO string in WorkflowMain.groovy: Add Zenodo DOI for pipeline after first release
  • pipeline_todos - TODO string in base.config: Check the defaults for all processes
  • pipeline_todos - TODO string in base.config: Customise requirements for specific processes.
  • pipeline_todos - TODO string in awsfulltest.yml: You can customise AWS full pipeline tests as required
  • schema_lint - Parameter input is not defined in the correct subschema (input_output_options)
  • system_exit - System.exit in WorkflowRnadeseq.groovy: System.exit(1) [line 67]
  • system_exit - System.exit in WorkflowMain.groovy: System.exit(1) [line 85]

❔ Tests ignored:

  • files_exist - File is ignored: .github/ISSUE_TEMPLATE/config.yml
  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: .github/workflows/push_dockerhub.yml
  • files_exist - File is ignored: assets/multiqc_config.yaml
  • files_exist - File is ignored: assets/nf-core-qbic-pipelines/rnadeseq_logo_light.png
  • files_exist - File is ignored: bin/markdown_to_html.r
  • files_exist - File is ignored: conf/test_full.config
  • files_exist - File is ignored: docs/images/nf-core-qbic-pipelines/rnadeseq_logo_dark.png
  • files_exist - File is ignored: docs/images/nf-core-qbic-pipelines/rnadeseq_logo_light.png
  • files_exist - File is ignored: lib/WorkflowQbic-pipelines/rnadeseq.groovy
  • nextflow_config - Config variable ignored: manifest.name
  • nextflow_config - Config variable ignored: manifest.homePage
  • nextflow_config - Config variable ignored: params.input
  • files_unchanged - File ignored due to lint config: LICENSE or LICENSE.md or LICENCE or LICENCE.md
  • files_unchanged - File ignored due to lint config: .github/CONTRIBUTING.md
  • files_unchanged - File ignored due to lint config: .github/ISSUE_TEMPLATE/bug_report.yml
  • files_unchanged - File ignored due to lint config: .github/ISSUE_TEMPLATE/config.yml
  • files_unchanged - File ignored due to lint config: .github/ISSUE_TEMPLATE/feature_request.yml
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: .github/workflows/branch.yml
  • files_unchanged - File ignored due to lint config: .github/workflows/linting.yml
  • 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: assets/sendmail_template.txt
  • files_unchanged - File ignored due to lint config: docs/README.md
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore or pyproject.toml

✅ Tests passed:

Run details

  • nf-core/tools version 2.8
  • Run at 2023-06-20 11:38:05


# Write cds assay table to file
write.table(counts(cds, normalized=T), paste("differential_gene_expression/gene_counts_tables/deseq2_table.tsv", sep=""), append=F, quote = F, sep = "\t", eol = "\n", na = "NA", dec = ".", row.names = T, col.names = T, qmethod = c("escape", "double"))
write.table(counts(cds, normalized=T), paste0("differential_gene_expression/gene_counts_tables/deseq2_table.tsv"), append=F, quote = F, sep = "\t", eol = "\n", na = "NA", dec = ".", row.names = T, col.names = T, qmethod = c("escape", "double"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could the output be named more explicitly? Like deseq2_normalized_gene_counts.tsv ?

Copy link
Collaborator Author

@WackerO WackerO Jun 20, 2023

Choose a reason for hiding this comment

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

Renaming to deseq2_library_scaled_gene_counts.tsv to not produce confusion with the rlog/vst normalization stuff that is done later. I'll also add this file to the file explanation in the expand/collapse box

assets/RNAseq_report.Rmd Outdated Show resolved Hide resolved
assets/RNAseq_report.Rmd Outdated Show resolved Hide resolved
assets/RNAseq_report.Rmd Outdated Show resolved Hide resolved
assets/RNAseq_report.Rmd Outdated Show resolved Hide resolved
assets/RNAseq_report.Rmd Outdated Show resolved Hide resolved
@@ -1808,18 +1835,41 @@ for (file in contrast_files){
mat$gene_name <- NULL
mat <- data.matrix(mat)

# Skip the heatmaps for CI tests
# Skip the heatmaps for CI tests because especially png() works only very unreliably for the heatmaps for some reason
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unreliably in terms of md5sums?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, unfortunately sometimes it also just does not work. I.e. Unable to start png device is thrown and the pipeline stops.

@@ -2140,10 +2187,12 @@ as.character(params$revision),
"` pipeline [^5], which was written using the nf-core template [@ewels2020nf]. For differential expression analysis, the read quantification data resulting from `",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this [^5] the citation number?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If so, it would be missing for the other citations like @love2014...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, it adds a superscript number and also links it to the citation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah sorry, no, some citations are just added without the reference thing. The reference is only done for links, i.e. [^5] moves the report down to this link: https://github.com/qbic-pipelines/rnadeseq. The other citations like love2014 are just listed without being clickable

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why this is not consistent with the clickable references?

assets/RNAseq_report.Rmd Outdated Show resolved Hide resolved
" `, `AnnotationDbi v", version_annotation,
"` and `", name_species, " v", version_annotation,
"` were employed. ", database_string, ".\n",
"Pathways were classified as enriched for those genes with padj <= ", pval_text, "."
Copy link
Collaborator

@SusiJo SusiJo Jun 20, 2023

Choose a reason for hiding this comment

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

Suggested change
"Pathways were classified as enriched for those genes with padj <= ", pval_text, "."
"Pathways were classified as enriched for genes with an adjusted p-value below ", pval_text, "."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

counter proposal: an adjusted p-value <= pval_test? I prefer having <= (or alternatively "less than or equal to") in the text so people don't wonder, even if that is an edge case

WackerO and others added 2 commits June 20, 2023 09:30
Co-authored-by: SusiJo <43847534+SusiJo@users.noreply.github.com>
Co-authored-by: SusiJo <43847534+SusiJo@users.noreply.github.com>
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
modules/local/report.nf Outdated Show resolved Hide resolved
WackerO and others added 3 commits June 20, 2023 10:05
Co-authored-by: SusiJo <43847534+SusiJo@users.noreply.github.com>
Co-authored-by: SusiJo <43847534+SusiJo@users.noreply.github.com>
Co-authored-by: SusiJo <43847534+SusiJo@users.noreply.github.com>
@SusiJo
Copy link
Collaborator

SusiJo commented Jun 20, 2023

If you are changing the parameter name of pval_threshold please also update the docs.

@WackerO
Copy link
Collaborator Author

WackerO commented Jun 20, 2023

If you are changing the parameter name of pval_threshold please also update the docs.

Param is now called adj_pval_threshold and this is reflected in usage.md, I assume that is what you meant? I'll ask @qbicStefanC about the renaming and if necessary do a follow-up PR to rename it back to pval_threshold

bin/Execute_report.R Outdated Show resolved Hide resolved
bin/Execute_report.R Outdated Show resolved Hide resolved
WackerO and others added 2 commits June 20, 2023 13:36
Co-authored-by: SusiJo <43847534+SusiJo@users.noreply.github.com>
Co-authored-by: SusiJo <43847534+SusiJo@users.noreply.github.com>
Copy link
Collaborator

@SusiJo SusiJo left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@WackerO WackerO merged commit 96a8583 into qbic-pipelines:dev Jun 21, 2023
@WackerO WackerO deleted the rlogic branch June 21, 2023 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants