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

Release 2.3 #233

Merged
merged 103 commits into from
Jun 13, 2024
Merged

Release 2.3 #233

merged 103 commits into from
Jun 13, 2024

Conversation

WackerO
Copy link
Collaborator

@WackerO WackerO commented May 7, 2024

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
  • 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).

WackerO and others added 30 commits October 10, 2023 16:15
Heatmap error, datasources, shorten filenames
Update RNAseq_report.Rmd for DE information
Copy link
Collaborator

@tillenglert tillenglert left a comment

Choose a reason for hiding this comment

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

LGTM (within my current knowledge of rnadeseq and R)! Still I had some minor comments 👍

assets/RNAseq_report.Rmd Show resolved Hide resolved
conf/test_custom_gmt.config Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
WackerO and others added 2 commits June 10, 2024 14:55
Add distance info for heatmaps, add heatmap tables, change volcano jitter to points
Copy link
Contributor

@d4straub d4straub left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Unfortunately 2 tests are failing.
Two tests (EditroConfig & Prettier) arent required any more and I adapted the settings accordingly.

Comment on lines +901 to +912
mqc_stats <- read.table(file = paste0(wd,"/QC/multiqc_data/multiqc_samtools_stats.txt"), header=TRUE, sep="\t")
columns <- c(
"Sample",
"raw_total_sequences",
"reads_mapped_percent",
"reads_duplicated_percent"
)
if (all(columns %in% colnames(mqc_stats))) {
mqc_version <- 'new_mqc'
} else {
stop("Could not find a suitable multiqc table; please provide a correct multiqc.zip file or omit the parameter --multiqc altogether.")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

That looks like a copy of the code in lines 887-898. Generally it seems not good to have the same code twice, maintenance and so. If possible, might be better to solve that differently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know, this is indeed not a nice code section but unfortunately I have to do this check twice, once inside a nested ifelse and once in a separate ifelse...

assets/methods_description_template.yml Outdated Show resolved Hide resolved
docs/usage.md Show resolved Hide resolved
@WackerO
Copy link
Collaborator Author

WackerO commented Jun 13, 2024

As https://nfcore.slack.com/archives/CE6SDBX2A/p1663077291333929 and https://nfcore.slack.com/archives/C02AL61NXPH/p1651131006534289 describe the same issue with the failing branch protection, and I could not find any solution there (and my attempts to fix it also failed), I'm going to ignore this error and merge.
Maybe it will resolve itself in the future but in this case, it is not a problem IMO.

@WackerO WackerO merged commit 062b03a into master Jun 13, 2024
34 of 35 checks passed
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.

4 participants