-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Map Ribo-TISH's colon-qualified ORF types on their location #12613
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
d4d18fc
4a6b135
a563217
45d5122
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -431,4 +431,51 @@ nextflow_process { | |
| } | ||
| } | ||
|
|
||
| test("ribotish composite ORF types map on the location, keeping the qualifier native") { | ||
|
|
||
| when { | ||
| process { | ||
| """ | ||
| input[0] = channel | ||
| .of( | ||
| 'Tid\tGid\tGenomePos\tTisType\tAALen\tFisherPvalue', | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have a real input we could use as a proper fixture in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes — real output with these tokens exists, so this needn't stay synthetic. The composite labels are what made a pipeline-level snapshot-recording run fail on genuine chr20 Ribo-TISH output, with this distribution: That is also why the module tests missed the gap — the existing fixture only contains bare tokens, exactly as you say. Two choices I would rather you made before I open the test-datasets PR, since both affect other modules:
Once you have picked, the order is test-datasets first, then this PR swaps the inline rows for the fixture. Happy to do both.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We try not to overwrite files in test-datasets, they should really be write-once (though it's patchily followed). Add a new one.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added as a new file in nf-core/test-datasets#2208, generated by running the pipeline on its own chr20 data in extended-ORF mode. Worth flagging: real output has seven colon-qualified forms, not the four we knew about -- 5'UTR:Known, 5'UTR:CDSFrameOverlap and Truncated:Known also occur, and all seven already resolve through the existing table. I will point the test at the fixture and drop the inline rows once that merges. |
||
| 't1\tg1\tchr1:1000-1100:+\tNovel:CDSFrameOverlap\t30\t0.001', | ||
| "t1\tg1\tchr1:1100-1200:+\t3'UTR:CDSFrameOverlap\t30\t0.001", | ||
| 't1\tg1\tchr1:1200-1300:+\tInternal:CDSFrameOverlap\t30\t0.001', | ||
| 't1\tg1\tchr1:1300-1400:+\tNovel:Known\t30\t0.001' | ||
| ) | ||
| .collectFile(name: 'composite.ribotish.pred.txt', newLine: true, sort: false) | ||
| .map { orfs -> [[id: 'sample1'], orfs, 'ribotish'] } | ||
| input[1] = channel | ||
| .of('chr1\ttest\texon\t1001\t2000\t.\t+\t.\tgene_id "g1"; transcript_id "t1";') | ||
| .collectFile(name: 'composite.gtf', newLine: true) | ||
| .map { gtf -> [[id: 'reference'], gtf] } | ||
| """ | ||
| } | ||
| } | ||
|
|
||
| then { | ||
| def lines = path(process.out.tsv[0][1]).text.readLines() | ||
| def header = lines.find { it.startsWith('orf_id') }.split('\t') as List | ||
| def data = lines.findAll { !it.startsWith('#') && !it.startsWith('orf_id') } | ||
| def i_class = header.indexOf('orf_class') | ||
| def i_native = header.indexOf('orf_type_native') | ||
| def classes = data.collect { it.split('\t')[i_class] } as Set | ||
| def natives = data.collect { it.split('\t')[i_native] } | ||
| def qualified = natives.findAll { it.contains(':') } | ||
| def provenance = lines.find { it.startsWith('# parser_columns:') } | ||
|
|
||
| assertAll( | ||
| // The colon-qualified tokens no longer abort the process: only the part | ||
| // before the colon is positional, so it alone selects the class. | ||
| { assert process.success }, | ||
| { assert classes == ['novel_u', 'dORF', 'intORF'] as Set }, | ||
| // The qualifier is dropped from orf_class but not lost. | ||
| { assert qualified.size() == natives.size() }, | ||
| { assert natives.contains('Novel:CDSFrameOverlap') }, | ||
| { assert provenance.contains('unmapped_orf_type=0') } | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reduce the AI historical narrative in the comments? I get this problem a lot, but comments should be stand-alone and represent the current state, not reference a previous state you're fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trimmed in
45d5122— comment-only across the three files, and no executable change:orfnormalise.py's token stream is identical apart from the docstring literal itself.classify()docstring: the paragraph describing what the previous code did is gone. It now states only that the part before the first colon is matched and that the full token survives inorf_type_native.CLASS_TOKENSheader: the Ribo-TISH narrative is replaced by the single constraint a future editor of that table actually needs —No key may contain a colon: classify() matches only the part before one.That invariant is load-bearing (a colon-bearing key would be silently truncated by the split), so I kept it, but moved it onto the table it constrains rather than leaving it as a justification in the docstring.meta.ymland the new test assertion: same treatment.One thing I left alone: there is a pre-existing instance of exactly this pattern a few lines above, from #12498 —
Substring matching mis-fired on the overlap forms, because "uorf" is a substring of "overlap_uorf". Happy to fold that in here if you'd like it gone now; I left it out so this PR stays scoped to comments it introduced itself.