Skip to content

Add custom/collectstats module - #12654

Merged
erikrikarddaniel merged 9 commits into
nf-core:masterfrom
erikrikarddaniel:custom-collectstats
Aug 12, 2026
Merged

Add custom/collectstats module#12654
erikrikarddaniel merged 9 commits into
nf-core:masterfrom
erikrikarddaniel:custom-collectstats

Conversation

@erikrikarddaniel

Copy link
Copy Markdown
Member

PR checklist

Related to nf-core/magmap#237 (not "Closes", it's phase 1 of a larger consolidation effort)

  • 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 module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Broadcast software version numbers to topic: versions
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test custom/collectstats --profile docker
      • nf-core modules test custom/collectstats --profile singularity
      • nf-core modules test custom/collectstats --profile conda

Description

New shared component to join per-sample read-processing statistics (trimming, decontamination, alignment, feature counting, and optionally taxonomy/function summaries) from several upstream tools into a single overall-stats table.

This is phase 1 of the consolidation plan discussed in nf-core/magmap#237: nf-core/magmap and nf-core/metatdenovo each independently maintain a local COLLECT_STATS module that turned out to be the same script, evolved separately from a common ancestor. Ported from magmap's version, the more hardened of the two (avoids shelling out via pipe(sprintf(...)) with sample-derived filenames in favour of Sys.glob()+readLines(); named-column read_tsv types instead of positional). Once this merges, both pipelines will adopt it in follow-up PRs to their own repos, removing their local duplicates.

It's a hand-rolled R aggregation script rather than a tool wrapper -- following the precedent already set by other custom/* modules (custom/rsemmergecounts, custom/matrixfilter, etc.).

Test data (small two-sample fixtures: Trim Galore reports, BBDuk logs, samtools idxstats, featureCounts-derived tables, an optional merge table) is in a separate PR to nf-core/test-datasets' modules branch: nf-core/test-datasets#2210. This PR depends on that one merging first -- CI here will fail to resolve the fixture paths until it does. Verified locally against both PRs' branches directly (all 3 nf-test cases pass with real Docker execution, including the optional-inputs-absent path), and separately verified the R aggregation logic by hand against the fixtures before either PR was opened.

erikrikarddaniel and others added 3 commits August 11, 2026 18:25
New shared component to join per-sample read-processing statistics
(trimming, decontamination, alignment, feature counting, and optionally
taxonomy/function summaries) from several upstream tools into a single
overall-stats table. Ported from nf-core/magmap's local
modules/local/collect/stats, the more hardened of two independently
evolved copies shared with nf-core/metatdenovo (avoids shelling out via
pipe(sprintf(...)) with sample-derived filenames; named-column read_tsv
types instead of positional) -- see nf-core/magmap#237 for the full
consolidation plan.

Test fixtures added separately to nf-core/test-datasets (modules
branch, nf-core/test-datasets#2210); this PR
depends on that one merging first.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Extends the featureCounts input list in all three test cases to
include Unassigned_NoFeatures/Ambiguity/MultiMapping/Unmapped
alongside CDS/rRNA, matching the fuller fixture set added to
nf-core/test-datasets#2210. Verified the underscore in these category
names doesn't interfere with the feature-type-from-filename parsing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@erikrikarddaniel
erikrikarddaniel marked this pull request as ready for review August 11, 2026 20:46

@sofiademmou sofiademmou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice ! ⭐

Just a few comments/questions. I mainly looked at the nf-core module aspect of this since i am not really an expert in R. You might want to ask someone more experienced in R and custom modules to be sure !

Comment thread modules/nf-core/custom/collectstats/main.nf
Comment thread modules/nf-core/custom/collectstats/meta.yml Outdated
Comment thread modules/nf-core/custom/collectstats/meta.yml Outdated
Comment thread modules/nf-core/custom/collectstats/tests/main.nf.test
Comment thread modules/nf-core/custom/collectstats/tests/main.nf.test Outdated
Comment on lines +109 to +110
path(process.out.overall_stats.get(0).get(1)).linesGzip
).match() }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add the version to the snapshot as well ?

Suggested change
path(process.out.overall_stats.get(0).get(1)).linesGzip
).match() }
path(process.out.overall_stats.get(0).get(1)).linesGzip
)
process.out.findAll { key, val -> key.startsWith("versions")}
.match() }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Confirmed, you understood it exactly right: yes, we do generate versions dynamically via packageVersion() etc., and yes, that's precisely why we've kept them out of the snapshot for these two tests -- if the underlying container ever gets rebuilt with a newer dplyr/readr/purrr, the snapshot would fail on a version bump that has nothing to do with any actual behavior change here. We hit exactly this problem before in our own pipeline's test suite, which is why we settled on this pattern there too. The stub test is different and can safely include versions in its snapshot, since those are hardcoded literals in the stub script, not queried at runtime -- they never drift. So I'd rather leave these two as-is. (Replied with some help from Claude Code.)

@sofiademmou sofiademmou Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you mean that the snapshot would fail if we update the container to a new version for a tool ? I think that should probably be reflected in the snapshot and we should update them then.

I also found this in the guidelines

Tests for modules MUST, at a minimum, run on the GitHub repository CI with a stub test that replicates the generation of (empty) output files and a versions file.

So I think the version should be included right ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You're right. Added.

erikrikarddaniel and others added 4 commits August 12, 2026 10:03
Co-authored-by: sofiademmou <97464042+sofiademmou@users.noreply.github.com>
Co-authored-by: sofiademmou <97464042+sofiademmou@users.noreply.github.com>
Also regenerates the stub test snapshot, stale since the earlier
sanitizeOutput() change (positional duplicate keys removed from the
captured process.out shape).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
erikrikarddaniel and others added 2 commits August 12, 2026 11:57
Per review feedback: the reviewer's point stands -- a version bump
causing a snapshot failure is a legitimate signal, not just noise,
and forces an explicit look/update rather than passing silently.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@sofiademmou sofiademmou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me ! 👍

@erikrikarddaniel
erikrikarddaniel added this pull request to the merge queue Aug 12, 2026
Merged via the queue into nf-core:master with commit 23e432f Aug 12, 2026
26 checks passed
@erikrikarddaniel
erikrikarddaniel deleted the custom-collectstats branch August 12, 2026 10:07
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