Conversation
These files conflicted with the patch and were reset wholesale to the nf-core/tools 4.0.3 template version, on the basis that they carry no pipeline-specific content. If this branch had local edits in them, those edits are gone — please review: .github/workflows/template-version-comment.yml
Remove vulnerable PR-comment artifact pattern
|
Warning Newer version of the nf-core template is available. Your pipeline is using an old version of the nf-core template: 4.0.2. For more documentation on how to update your pipeline, please see the Synchronisation documentation. |
|
dialvarezs
suggested changes
Jul 28, 2026
dialvarezs
left a comment
dialvarezs
left a comment
Member
There was a problem hiding this comment.
I started reviewing in the Pseudo PR and forgot to copy the comments here before publishing the review, sorry!
#6
❌ nf-test failed with latest Nextflow versionNote Tests with Nextflow's latest version failed but it will not cause a CI workflow failure.
See the full run for details. |
chriswyatt1
reviewed
Jul 30, 2026
| <p>The pipeline was executed with Nextflow v${workflow.nextflow.version} (<a href="https://doi.org/10.1038/nbt.3820">Di Tommaso <em>et al.</em>, 2017</a>) with the following command:</p> | ||
| <pre><code>${workflow.commandLine}</code></pre> | ||
| <p>${tool_citations}</p> | ||
| <p>Tools used in the workflow included: FastQC (Andrews 2010), Cutadapt (Martin 2011), UMI-tools (Smith <em>et al.</em>, 2017), Bowtie (Langmead <em>et al.</em>, 2009), Bowtie2 (Langmead & Salzberg 2012), SAMtools (Danecek <em>et al.</em>, 2021), RNAFramework (Incarnato <em>et al.</em>, 2018), and MultiQC (Ewels <em>et al.</em>, 2016).</p> |
There was a problem hiding this comment.
minor comment: maybe add some more of the main programs used
chriswyatt1
reviewed
Jul 30, 2026
| SAMPLE_PAIRED_END,/path/to/fastq/files/AEG588A1_S1_L002_R1_001.fastq.gz,/path/to/fastq/files/AEG588A1_S1_L002_R2_001.fastq.gz | ||
| SAMPLE_SINGLE_END,/path/to/fastq/files/AEG588A4_S4_L003_R1_001.fastq.gz, | ||
| sample,sample_id,fastq_1,fastq_2,method,principle,chemical,RT_enzyme,sample_group,condition,replicate,organism,pH,adapter_3p,adapter_5p,umi_pattern | ||
| HEK293T_SHAPE_treated_r1,GSM000001,/data/HEK293T_SHAPE_treated_r1.fastq.gz,,SHAPE,MaP,NAI,M-MLV,HEK293T,treated,1,Homo sapiens,,AGATCGGAAGAGC,,NNNNNNNN |
There was a problem hiding this comment.
Will nextflow work with:
--organism Homo sapiens
Should it not be something like:
--organism "Homo sapiens"
There was a problem hiding this comment.
I got confused here, because i saw organism in the nextflow.config. So its fine in the samplesheet like this
chriswyatt1
reviewed
Jul 30, 2026
| "enum": ["treated", "untreated", "denatured"], | ||
| "meta": ["condition"], | ||
| "description": "Sample condition for rf-norm grouping (treated / untreated / denatured). Samples are normalised only against rows with the same sample_group and replicate. Untreated requires a matching treated sample; denatured requires matching treated and untreated samples.", | ||
| "help_text": "TODO (rnacentral-probing-metadata-main): merge_metadata.py must be updated to populate this column from the YAML 'conditions' field. Until then, samples without a condition are treated as 'treated'." |
chriswyatt1
reviewed
Jul 30, 2026
| @@ -0,0 +1,76 @@ | |||
| #!/usr/bin/env python3 | |||
| """Average per-replicate merged WIG files; pass through unchanged when only one replicate. | |||
|
|
|||
There was a problem hiding this comment.
Same for many of the bin scripts. Or where this code has come from if imported from another package
There was a problem hiding this comment.
Could be nice a little more code notation in all the bin scripts, to know what the main sections are doing.
chriswyatt1
reviewed
Jul 30, 2026
There was a problem hiding this comment.
I think its fine to have this in conf. But just looking at the nextflow.config, that points to here. If there is already a nf-core institutional config for ebi codon slurm, maybe in nextflow config it doesn't need to specify all the cluster settings. Use like -profile ebi_codon_slur,codon ?
https://nf-co.re/configs/ebi_codon_slurm/
chriswyatt1
reviewed
Jul 30, 2026
|
|
||
| withName: CUTADAPT_RTSTOP { | ||
| ext.args = { | ||
| def default_adapter = 'AGATCGGAAGAGC' |
There was a problem hiding this comment.
SHould there be fixed parameters in the modules config? Maybe this is fine, just wanted to check
chriswyatt1
reviewed
Jul 30, 2026
| // No node-local scratch: like RFCOUNT_STAR, the per-reference BAM split can exhaust the | ||
| // compute node's local scratch ($TMPDIR) with ENOSPC. | ||
| scratch = false | ||
| container = params.rnaframework_container |
There was a problem hiding this comment.
Do we need to allow the user to change the container used for this step? If so then I think this is fine. But if this is unnecessary, I would remove it.
chriswyatt1
reviewed
Jul 30, 2026
| ```text | ||
| <outdir>/ | ||
| ├── fastqc/ (raw and post-trimming reads) | ||
| ├── cat/ (if multi-lane FASTQ merging occurs) |
chriswyatt1
reviewed
Jul 30, 2026
|
|
||
| ### Reference resolution | ||
|
|
||
| The `organism` value (per-sample column or `--organism`) is used to resolve and download the reference automatically. Latin binomials (`Homo sapiens`) are preferred, but a small set of shorthands are also accepted: `human`, `mouse`, `rat`, and `yeast`. |
There was a problem hiding this comment.
OK,. so I think its here, it should be mentioned to use quotes, when setting organism as a flag
chriswyatt1
reviewed
Jul 30, 2026
|
|
||
| | Profile | Description | | ||
| | -------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `test` | Minimal test using the STAR genome-alignment route. Uses human mitochondrial chromosome (MT-RNR1) test data; no other parameters needed. | |
There was a problem hiding this comment.
test_full missing maybe if i remember corrcet
|
We need to convert all the local modules to use the topics channel : |
chriswyatt1
reviewed
Jul 30, 2026
| ] | ||
| } | ||
|
|
||
| process GTF_SANITIZE { |
There was a problem hiding this comment.
Should this be its own module? There are two processes in this module script
chriswyatt1
reviewed
Jul 30, 2026
|
|
||
| input: | ||
| tuple val(meta), path(wig) | ||
| path merge_script |
There was a problem hiding this comment.
I don't think we need this. The merge script is in the bin. So, we should remove this input variable, and then call the script in the script scope, which will be found, if the python script is in the bin, unless there is a reason to keep?
chriswyatt1
reviewed
Jul 30, 2026
|
|
||
| input: | ||
| tuple val(meta), val(accessions) | ||
| path ncbi_fasta_script |
There was a problem hiding this comment.
Same here. Unless there is some reason to do this?
chriswyatt1
reviewed
Jul 30, 2026
| output: | ||
| tuple val(meta), path("${prefix}_r2dt/"), optional: true, emit: diagrams | ||
| tuple val(meta), path("r2dt_drawn_ids.txt"), emit: drawn_ids | ||
| tuple val(meta), path("${prefix}_r2dt.log"), emit: log |
There was a problem hiding this comment.
Minor issue, but indentation is nto quite right in a few of the local modules.
If a lot of local modules are only using python, then maybe even if just one of them is send to the topic channels, it would then work. But maybe its not too complicated to adjust them all? |
chriswyatt1
reviewed
Jul 30, 2026
| principle = null | ||
| chemical = null | ||
| RT_enzyme = null | ||
| pH = null |
There was a problem hiding this comment.
Will all of these params be needed. It seems a bit of a replication of the samplesheet. If they want all organism "homo sapiens"., maybe this should be noted in the samplesheet only. What happens if a user sets this twice?
chriswyatt1
reviewed
Jul 30, 2026
There was a problem hiding this comment.
Hi @Vicbeg .
Really great, its very clear and I have gone through most of the minor files (Fernando will focus on the subworkflows/pipeline logic). I am also trialing the pipeline a few times to find any bugs.
No major issues. You will see in all the comments to the specific lines above.
A lot are a bit nit picky (and things I now realise genomeqc needs to fix), so of course feel free to reject the suggestions :)
We both need to update to nf-core template too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR prepares the first release (v1.0.0) of nf-core/rnastructurome, merging
devintomaster.What this pipeline does
nf-core/rnastructurome analyses RNA chemical-probing data (e.g. DMS / SHAPE) to
infer RNA secondary structure. It covers the full path from raw reads to
reactivity-annotated structures:
automatic route selection for NCBI/Ensembl references (incl. viral & prokaryote)
rf-count/rf-count-genome, strandednessinference,
rf-normnormalisation, replicate handling andrf-correlateQCrf-fold(with replicate consensus),rf-jackknifecalibration, ViennaRNA
genome-browser tracks, and a MultiQC summary report
PR checklist
nf-core pipelines lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile debug,test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).