Decouple orf_class from ORF length across the custom/orf* modules

[FelixKrueger]

custom/orfnormalise overwrote orf_class with smORF for every ORF at or under 100 aa, so a short uORF, a short downstream ORF, a short truncated CDS variant and a short novel intergenic ORF were indistinguishable in exactly the size range microprotein work cares about — and because custom/orfmerge keys its clustering strategy on orf_class and custom/orfcollapse scoped its peptide dedup to that value, the length override also decided how an ORF merged across callers. orf_class is now purely positional and length moves to a separate is_smorf flag driven by a new --smorf-max-aa (default 100, which reproduces the previous membership exactly), and the vocabulary gains uoORF, doORF and intORF — classes the callers were already reporting and the substring matching was folding away. This is a breaking change to orf_class values; it needs the refreshed fixture from nf-core/test-datasets#2193, which is merged.

AI-assisted detail — keep or bin as you see fit

Why the classes were being lost

Tokens were matched by substring, so overlap_uORF matched the uorf rule. Verified against each caller's source:

caller	native label	was	now
RiboCode	Overlap_uORF / Overlap_dORF	uORF / dORF	uoORF / doORF
ribotricer	overlap_uORF / overlap_dORF	uORF / dORF	uoORF / doORF
RiboCode, Ribo-TISH	internal / Internal	other	intORF
PRICE	uoORF, iORF, orphan	uORF, other, other	uoORF, intORF, novel_u

Matching is now exact per caller — every vocabulary is a closed enum — and the count of tokens matching no entry is reported as unmapped_orf_type on the # parser_columns: provenance line, so an upstream label change fails loudly instead of landing in the wrong class. It is 0 for all five callers on the chr20 fixtures.

ribotricer's internal deliberately stays other: it is check_orf_type()'s terminal fall-through rather than a frame-tested call, so intORF would assert a relationship the tool never tested.

Why orf_class had to leave the merge key

Callers disagree on class for the same ORF, and Ribo-TISH cannot agree even in principle — tisType() tests only the start position, so 5'UTR covers both uORF and uoORF. Keying on class emits one row per disagreeing caller. orfmerge now partitions by clustering strategy with class absent from every grouping key, and representative() resolves the disagreement via CLASS_SPECIFICITY (most specific first). There is a new test where four callers report the same ORF as Overlap_uORF, overlap_uORF, uoORF and 5'UTR and collapse to one uoORF row.

Two further clustering fixes fell out of that:

• Overlap is measured on summed exon-block intersection, not the outer genomic span. For a spliced ORF the span is mostly intron, so span-based overlap is insensitive to real differences — two ORFs differing by a quarter of their codons could score >0.99.
• Linkage is complete, not single. Single linkage chained distinct ORFs through an intermediate overlapping both; on a synthetic fixture one row absorbed a uORF, a uoORF, a doORF, an internal ORF and an extended CDS.

canonical_cds is grouped by transcript and then overlap-clustered within it, so a short truncated variant is no longer folded into the full-length CDS and emitted as the longest member only. Transcript-anchored classes keep the exact-span key: overlap clustering merges nested ORFs, and a 100 aa uORF covering 84% of its CDS folded into the CDS when I tried it. Separating "same ORF, fuzzy bounds" from "different ORF, mostly nested" needs a shared-stop-codon test, which I have left for a follow-up. The consequence — two callers whose short-ORF bounds differ by a few nt give one row each — is pinned by a test.

Unknown orf_class values and unassigned rows now abort rather than being dropped from the catalogue silently, which the previous closed enumeration did.

orfcollapse

Eligibility is derived from aa_length plus --smorf-max-aa rather than a propagated flag, so the collapse scope cannot drift from the published is_smorf, and the module aborts if the columns it depends on are missing. The survivor of a cross-class peptide fold now prefers the more specific class: previously it was chosen by length alone, so a 40 aa annotated CDS was deleted by a 60 aa novel ORF sharing its peptide and the MultiQC table reported zero canonical_cds.

New column

orf_type_native carries each caller's own ORF-type label through to the catalogue (unioned across a cluster), so every mapping decision above is auditable without re-running callers.

Testing

Nine tests on orfnormalise, seven on orfmerge, two on orfcollapse. New: the is_smorf boundary at --smorf-max-aa and at +1 with orf_class unchanged; a non-default --smorf-max-aa override; the four-caller class-disagreement merge; canonical_cds variant separation; and the pinned short-ORF de-merge. The two synthetic tests from #12495 gain the two new columns — without them the new presence guard aborts.

Snapshots are re-recorded. Content md5s come from a local run, which is byte-identical to CI for deterministic outputs; versions.yml md5s are kept from the committed snapshots, since those carry the container's interpreter rather than my host's. I could not run the module containers locally (no DNS in my Docker VM), so every test was executed container-free and CI is the authority on that last piece.

[pinin4fjords]

AI-assisted review by Codex, on behalf of @pinin4fjords.

I found three behavioural bugs. I reproduced each one at the current commit and tested the complete fix on arm64 and x86_64.

These parts span multiple locations:

• Add modules/nf-core/custom/orfnormalise/tests/nextflow.config and load it once from main.nf.test:

params.module_args = ""

process {
    withName: "CUSTOM_ORFNORMALISE" {
        ext.args = params.module_args
    }
}

Set module_args in both override tests, then delete both one-off config files.

• Add three regression tests: flag-based collapse eligibility, exact structures across strategy buckets, and failure on an unknown ORF type. No snapshot changes are needed.
• Update the orfcollapse.py docstring and meta.yml. is_smorf controls eligibility. Representative order is class specificity, length, then orf_id.
• For cross-strategy merging, group exact structures first, choose one representative proxy, run the existing strategy clustering, then restore the proxy members.

Comment added by Codex

[pinin4fjords]

[Medium] Remove the duplicate smORF threshold

is_smorf already reaches this process. A second threshold can disagree with it: when the normaliser uses 150 aa, collapse still excludes rows above 100 aa.

Suggested change

	parser.add_argument(
	"--smorf-max-aa",
	type=int,
	default=100,
	help="Maximum aa_length eligible for peptide-level collapse (default: 100)",
	)
	args = parser.parse_args(shlex.split("${args}"))
	if args.smorf_max_aa < 1:
	sys.exit(f"orfcollapse: --smorf-max-aa must be >= 1, got {args.smorf_max_aa}")
	args = parser.parse_args(shlex.split("${args}"))

Comment added by Codex

[FelixKrueger]

Kept the flag, but made the disagreement impossible rather than removing the cross-check.

is_smorf is now the only thing that decides eligibility (orfcollapse.py:219), and orfcollapse.py:209-215 aborts if the flag and --smorf-max-aa ever disagree — naming the offending orf_ids and telling the caller to pass the same value to both processes. So the 150-vs-100 case you describe fails loudly instead of silently excluding rows.

The reason for keeping it: deleting the argument removes the disagreement but also removes the check, and the module stays usable standalone outside the pipeline. Happy to drop it entirely if you'd rather orfcollapse take the catalogue's word unconditionally.

[pinin4fjords]

[Medium] Require and validate is_smorf

Collapse now depends on the flag from the merged catalogue, so it should fail clearly if that column is missing or contains anything except 0 or 1.

Suggested change

	# The collapse scope is derived from these two columns, so a silent rename
	# upstream must abort rather than quietly collapse nothing.
	missing = [c for c in ("orf_class", "aa_length") if c not in header]
	if missing:
	sys.exit(f"orfcollapse: catalogue is missing required column(s) {missing}")
	unknown = sorted(set(catalogue["orf_class"]) - set(CLASS_ORDER))
	if unknown:
	sys.exit(f"orfcollapse: unknown orf_class value(s) {unknown}; update CLASS_ORDER")
	missing = [c for c in ("orf_class", "aa_length", "is_smorf") if c not in header]
	if missing:
	sys.exit(f"orfcollapse: catalogue is missing required column(s) {missing}")
	unknown = sorted(set(catalogue["orf_class"]) - set(CLASS_ORDER))
	if unknown:
	sys.exit(f"orfcollapse: unknown orf_class value(s) {unknown}; update CLASS_ORDER")
	invalid_smorf = sorted(set(catalogue["is_smorf"]) - {"0", "1"})
	if invalid_smorf:
	sys.exit(f"orfcollapse: invalid is_smorf value(s) {invalid_smorf}; expected 0 or 1")

Comment added by Codex

[FelixKrueger]

Done — orfcollapse.py:178-188 now has three checks before anything is written: missing required columns (orf_class, aa_length, is_smorf), unknown orf_class values, and is_smorf values outside {0, 1}.

[pinin4fjords]

[Medium] Use the flag already in the catalogue

Use the decision made by orfnormalise. I tested a 120-aa row marked 1 and a 50-aa row marked 0. Only the flagged pair collapsed.

Suggested change

	# Eligibility is derived here from aa_length rather than read from a
	# propagated flag, so the scope cannot silently drift from --smorf-max-aa.
	def is_small(row):
	try:
	aa = int(row.get("aa_length") or 0)
	except ValueError:
	return False
	return 0 < aa <= args.smorf_max_aa
	clusters = defaultdict(list)
	for r in rows:
	if r.get("orf_class") == SMORF_CLASS:
	if is_small(r):
	clusters = defaultdict(list)
	for r in rows:
	if r["is_smorf"] == "1":

Comment added by Codex

[FelixKrueger]

Done — orfcollapse.py:219 clusters on r.get("is_smorf") == "1", so the decision comes from orfnormalise rather than being re-derived here.

[pinin4fjords]

[Medium] Fail if a caller adds a new ORF type

Unknown tokens become other, which can select the wrong merge strategy. Fail before writing outputs. The tested error includes the token and count.

Suggested change

	write_outputs(OUT_BED, OUT_TSV, bed_lines, tsv_rows, resolved_columns, unmapped, args.smorf_max_aa)
	if unmapped:
	details = ", ".join(f"{token!r} ({count})" for token, count in sorted(unmapped.items()))
	sys.exit(f"orfnormalise: unmapped ORF type value(s): {details}")
	write_outputs(OUT_BED, OUT_TSV, bed_lines, tsv_rows, resolved_columns, unmapped, args.smorf_max_aa)

Comment added by Codex

[FelixKrueger]

Done — orfnormalise.py:1017 exits before write_outputs, with the token and its count in the message.

[pinin4fjords]

[Low] Set the override from the test

The shared config in the review body exposes module_args, so this test needs no separate config file. I tested the same pattern for both overrides.

Suggested change

	config "./smorf_max_aa.config"
	when {
	when {
	params {
	module_args = "--smorf-max-aa 150"
	}

Comment added by Codex

[FelixKrueger]

Done — main.nf.test:364 sets module_args = "--smorf-max-aa 150" from the test, and the --score-field case at :289 uses the same pattern. The separate config file is gone.

[pinin4fjords]

[Low] Update the docs to match the code

The docs say that the longest ORF wins, but the code uses class specificity first. They should also name is_smorf as the eligibility source.

Comment added by Codex

[FelixKrueger]

Done — orfcollapse/meta.yml now reads "the representative is chosen by class specificity, then longest aa_length, then orf_id", and names the catalogue's is_smorf flag as the eligibility source.

[pinin4fjords]

[Medium] Merge exact structures before applying class rules

An exact ORF can receive different classes from different callers. Group those rows first, use the most specific representative to select the strategy, then restore every member after clustering.

Suggested change

	by_class = defaultdict(list)
	for r in rows:
	by_class[r.get("orf_class", "other")].append(r)
	unknown = sorted(set(by_class) - set(CLASS_ORDER))
	if unknown:
	sys.exit(f"orfmerge: unknown orf_class value(s) {unknown}; update CLASS_ORDER")
	clusters = []
	# canonical CDS: one per transcript by definition - collapse by (tid, strand).
	clusters.extend(group_by(by_class.get("canonical_cds", []), lambda r: (r.get("transcript_id") or "", r["strand"])))
	# uORF/dORF/other: a transcript can host multiple distinct ones, so
	# additionally key on the outer span to keep them separate.
	for cls in ("uORF", "dORF", "other"):
	clusters.extend(
	group_by(
	by_class.get(cls, []),
	lambda r: (r.get("transcript_id") or "", r["strand"], int(r["start"]), int(r["end"])),
	)
	# Not transcript-anchored: one reciprocal-overlap pass over the union of
	# these classes, so calls disagreeing on class still reach one cluster.
	non_anchored = [r for cls in OVERLAP_CLASSES for r in by_class.get(cls, [])]
	clusters.extend(cluster_by_reciprocal_overlap(non_anchored, frac=args.reciprocal_overlap))
	# One annotated CDS per transcript, so the span stays out of the key --
	# but grouping on (transcript_id, strand) alone would fold a short
	# truncated variant into the full-length CDS and emit only the longest, so
	# cluster on exonic overlap within the transcript.
	loose = [r for cls in LOOSE_CLASSES for r in by_class.get(cls, [])]
	for grp in group_by(loose, lambda r: (r.get("transcript_id") or "", r["strand"])):
	clusters.extend(cluster_by_reciprocal_overlap(grp, frac=args.reciprocal_overlap))
	# Transcript-anchored and non-unique per transcript, so the outer span
	# joins the key. Overlap clustering cannot be used here: it merges nested
	# ORFs, folding a uORF that covers most of the CDS into the CDS itself.
	keyed = set(OVERLAP_CLASSES) | set(LOOSE_CLASSES)
	anchored = [r for cls, rows_c in by_class.items() if cls not in keyed for r in rows_c]
	clusters.extend(
	group_by(
	anchored,
	lambda r: (r.get("transcript_id") or "", r["strand"], int(r["start"]), int(r["end"])),
	)
	# novel_u / smORF: not transcript-anchored - reciprocal-overlap clustering.
	for cls in ("novel_u", "smORF"):
	clusters.extend(cluster_by_reciprocal_overlap(by_class.get(cls, []), frac=args.reciprocal_overlap))
	)
	exact_groups = group_by(rows, lambda r: (r["chrom"], r["strand"], tuple(r["_blocks"])))
	merge_rows = []
	for members in exact_groups:
	proxy = dict(representative(members))
	proxy["_members"] = members
	merge_rows.append(proxy)
	by_class = defaultdict(list)
	for r in merge_rows:
	by_class[r.get("orf_class", "other")].append(r)
	unknown = sorted(set(by_class) - set(CLASS_ORDER))
	if unknown:
	sys.exit(f"orfmerge: unknown orf_class value(s) {unknown}; update CLASS_ORDER")
	proxy_clusters = []
	non_anchored = [r for cls in OVERLAP_CLASSES for r in by_class.get(cls, [])]
	proxy_clusters.extend(cluster_by_reciprocal_overlap(non_anchored, frac=args.reciprocal_overlap))
	loose = [r for cls in LOOSE_CLASSES for r in by_class.get(cls, [])]
	for grp in group_by(loose, lambda r: (r.get("transcript_id") or "", r["strand"])):
	proxy_clusters.extend(cluster_by_reciprocal_overlap(grp, frac=args.reciprocal_overlap))
	keyed = set(OVERLAP_CLASSES) | set(LOOSE_CLASSES)
	anchored = [r for cls, rows_c in by_class.items() if cls not in keyed for r in rows_c]
	proxy_clusters.extend(
	group_by(
	anchored,
	lambda r: (r.get("transcript_id") or "", r["strand"], int(r["start"]), int(r["end"])),
	)
	)
	clusters = [
	[member for proxy in cluster for member in proxy["_members"]]
	for cluster in proxy_clusters
	]

Comment added by Codex

[FelixKrueger]

Done — orfmerge.py:445-449 groups rows by exact structure (chrom, strand, blocks) into a proxy carrying _members, selects the strategy from the most specific representative, and :480 restores every member after clustering. So callers that disagree on class for the same structure merge into one row.

[FelixKrueger]

Thanks — all three reproduce, and they're fixed in 23abd3f. One I've solved differently from the suggestion, so flagging that up front.

Collapse threshold. Rather than dropping --smorf-max-aa, orfcollapse now takes eligibility from is_smorf and keeps the threshold solely to re-derive it, aborting when the two disagree. The misconfiguration you found then fails loudly instead of silently narrowing the scope — 13 rows on our synthetic cohort — without letting a row's flag contradict its own aa_length, which is what deriving is_smorf from the row's own length requires. The consequence is that a 120-aa row marked 1 is now an error rather than supported behaviour, so the regression test asserts the abort rather than the collapse. Happy to go all the way to your version if you'd rather the flag simply win.

Cross-strategy merging turns out to be byte-identical on every existing fixture: the chr20 data contains no pair of callers that agree on exact structure but disagree on class across a strategy boundary. So there are no snapshot changes, and a synthetic test pins the new behaviour.

Detail (AI-assisted)

Unmapped ORF types — taken as suggested. Explicit other mappings (PRICE intronic, ribotricer internal) are unaffected, since they report as matched and never reach the counter, so a non-zero count genuinely means a token no one has seen.

is_smorf required and validated — added, including the 0/1 check.

Exact-structure pre-grouping — rows sharing (chrom, strand, blocks) are grouped first and clustered as a single proxy chosen by class specificity, then the members are restored, so the row-conservation guard still counts real rows. Note the proxy key deliberately excludes transcript_id: including it would preserve the anchored bucket's semantics but fail to fix the main case, since PRICE orphan rows frequently have no transcript_id while Ribo-TISH's uORF does. Two ORFs with identical exonic structure on different overlapping transcripts will now merge.

Docs — orfcollapse's docstring and meta.yml both claimed the longest ORF wins, but merge_members ranks by class specificity first; both now say so and name is_smorf as the eligibility source. One more in the same vein that wasn't in the review: orfmerge's meta.yml described the novel_u pass as reciprocal overlap "on the outer genomic span", whereas cluster_by_reciprocal_overlap measures summed exon-block intersection — its own docstring explains why the span is the wrong measure for a spliced ORF.

Test config — shared orfnormalise/tests/nextflow.config exposing params.module_args, set from both override tests, and the two one-off config files deleted. The same pattern was added for orfcollapse so its new test can set --smorf-max-aa.

Regression tests — identical structures merging across strategy buckets (novel_u + uORF at one structure, asserting one row and both callers); an unmapped ORF type aborting and naming the token; a --smorf-max-aa that disagrees with is_smorf aborting. All three are assertion-only, so no snapshot entries.

Verification — 21/21 across the three modules, run locally container-free. Content md5s matched the committed snapshots on the first run; only versions.yml differed, since it carries the local interpreter version, so the snapshots are unchanged from what CI recorded.

Known gap — the new missing-column and invalid-is_smorf guards aren't covered by a test yet; both need an inline catalogue fixture. Say if you'd like them before merge.

[pinin4fjords]

AI-assisted review by Codex on behalf of @pinin4fjords.

Most fixes work. The x86_64 module and subworkflow runs produced consistent TSV, BED, FASTA, mapping, consensus and MultiQC outputs, including the 150-aa threshold path. One class-validation edge case remains, and the metadata should match the fail-fast behaviour. Details inline.

Comment added by Codex

[pinin4fjords]

[Low] Describe the fail-fast behaviour

Unmatched tokens abort before outputs are written, so a non-zero count cannot appear in the provenance line.

Suggested change

	ORF-type tokens are matched exactly (casefolded) against each caller's closed
	vocabulary; the count of tokens matching no entry is reported as
	`unmapped_orf_type` on the `# parser_columns:` provenance line.
	ORF-type tokens are matched exactly (casefolded) against each caller's closed
	vocabulary. Tokens matching no entry cause the process to fail; successful
	outputs report `unmapped_orf_type=0` on the `# parser_columns:` provenance line.

Comment added by Codex

[FelixKrueger]

Applied verbatim — thanks.

[pinin4fjords]

[Medium] Validate classes before selecting representatives

An unknown class can be hidden when its structure matches a known class. Validate the original rows before exact grouping.

Suggested change

	unknown = sorted({row.get("orf_class", "other") for row in rows} - set(CLASS_ORDER))
	if unknown:
	sys.exit(f"orfmerge: unknown orf_class value(s) {unknown}; update CLASS_ORDER")

Comment added by Codex

[FelixKrueger]

Done — orfmerge.py:440 validates orf_class across the original rows, before exact grouping, so an unknown class can't hide behind a known-class structure.

[FelixKrueger]

Both taken, in d121556.

The class validation is a real regression from my exact-structure grouping — good catch. representative() ranks an unknown class last, so at a shared structure the known class always won the proxy and the unknown one escaped the guard, after which write_catalogue re-emitted the row under the known class: exactly the silent swallow the guard exists to prevent. Validation now runs on the input rows before grouping, and the new test does fail against the previous commit, so it pins the regression rather than just the fix.

meta.yml wording taken as suggested. 19/19 locally, snapshots unchanged.