Keep SEQid to a single field in the output tables - #46
Open
alexlancaster wants to merge 1 commit into
Open
Conversation
Sequence names are copied verbatim from the FASTA header into the SEQid column of both output tables, which are tab-delimited. A tab inside a header therefore split SEQid across several fields and shifted every column after it. Nothing failed: the header row kept its 41 columns while affected data rows gained one field per tab, so anything reading the table by column index silently read values from the wrong columns. MitoCarta and several other curated FASTA distributions ship tab-separated headers, which is how this is usually met. A related inconsistency came from fastareader.nextfasta(), which discarded the result of its trim(). The first record of a file takes its name from hasmorefastas(), which does trim, while later records took it from nextfasta(), which did not. A file whose headers carried trailing whitespace could therefore produce a different column count for its first record than for the rest. Map tabs, newlines and other control characters in the name to spaces and trim it, so SEQid is always exactly one field, and assign the trimmed name in nextfasta(). Names without control characters or surrounding whitespace are unchanged, embedded spaces included, so the MOT3 golden output is byte-identical. cli/tests/test_seqid_field.sh covers a tab inside a header, agreement between the tab- and space-separated forms of the same header, trailing tabs and spaces across a multi-record file, the per-residue table under -p, and that ordinary names are left alone. Five of its six checks fail without this change. Wired into cli.yml alongside the MOT3 and input-validation tests.
Contributor
Author
|
hey @oliverking since this touches the Java code, could you take a look at this? it looks fine to me. there are unit tests that check that it covers the cases, it doesn't change any of the existing test outputs |
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
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.
recreated from #41 & #42 as the fork was deleted.
summary below is by @ssiddhantsharma:
Sequence names are copied verbatim from the FASTA header into the
SEQidcolumn, and both output tables are tab-delimited. A tab inside a header therefore splitsSEQidacross several fields and shifts every column after it.Nothing errors. The header row keeps its 41 columns while affected data rows gain one field per tab, so anything reading the table by column index silently reads the wrong values:
MitoCarta and several other curated FASTA distributions ship tab-separated headers (
>accession<TAB>GeneID:nnn<TAB>SYMBOL), which is how this is usually met. ReadingCOREscoreby index off such a table returns whatever sits two columns to its left.A related inconsistency comes from
fastareader.nextfasta(), which discards the result of itstrim():The first record of a file takes its name from
hasmorefastas(), which does trim, while later records took it fromnextfasta(), which did not. So a file whose headers carry trailing whitespace could produce a different column count for its first record than for the rest:The change
A small
tsvSafeName()helper maps tabs, newlines and other control characters in the name to spaces and trims it, applied where the name is written into each of the two tables.nextfasta()now assigns the trimmed name.Sanitising at the point of output rather than at capture keeps
-pprint-list matching untouched, and also covers names supplied through a print list rather than the FASTA.Names with no control characters and no surrounding whitespace are returned unchanged, embedded spaces included, so the MOT3 golden output is byte-identical and
tests/test_mot3.shpasses unmodified.Tests
cli/tests/test_seqid_field.sh, following the existingtest_common.shconventions, covers:-p allmatches between the two header formsFive of the six fail without the source change. Wired into
cli.ymlbeside the MOT3 and input-validation tests.Verified locally on Java 17:
test_mot3.sh,test_input_validation.shandtest_seqid_field.shall pass, and the Python wrapper suite passes against the patched jar (23 passed withPLAAC_JARpointed at it).