fix(bed): read BED files through htslib so gzip and bgzip work (#230) - #240
Merged
Conversation
bedData::bedData() read with an ifstream, so a compressed BED could not be parsed. The fopen existence check succeeded on a .bed.gz, the first line then decoded to binary garbage, and parse_coord() reported an invalid coordinate -- naming the wrong cause, since the coordinate was not malformed. ERROR() exits, so the try/catch around the -b constructor never ran. Replace the ifstream with hts_open() plus hts_getline() on a kstring_t, which decodes plain, gzip, and bgzip transparently and detects the encoding from the file's leading bytes rather than its extension. A read failure below EOF is now reported rather than being mistaken for a short file, so a truncated compressed BED no longer parses as a partial region set. The malformed-coordinate ERROR paths are unchanged. htslib was already a link dependency; bcf_open() reads the VCFs.
A quoted include is resolved against the including file's own directory before any -I path, so "htslib/bgzf.h" in tests/unit/src/test_helpers.cpp searched tests/unit/src/htslib/ and stopped. It built locally only because Homebrew's htslib is on the default search path; CI copies the headers into src/ instead of installing them, so the test build failed to find bgzf.h. Add -I$(SRC) to TEST_CXXFLAGS, giving the test objects the same htslib resolution the src objects get for free by living in src/. Verified both ways: with the headers copied to src/htslib (the CI layout) they resolve from there, and with that directory absent they fall back to the system path. CXXFLAGS is untouched, since it must stay identical to src/Makefile.
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.
Note
Authorship: the content below was drafted by Claude Opus 5 (an AI coding agent) and
filed via
ghunder @TimD1-bot, a bot account operated by @TimD1. It reflects theagent's analysis, not a statement authored by @TimD1.
Summary
bedData::bedData()read with anstd::ifstream, so a compressed BED could not be parsed — andit failed in the worst possible way. The constructor's
fopenexistence check succeeded on a.bed.gz, the first line then decoded to binary garbage, andparse_coord()reported:which names the wrong cause: the coordinate is not malformed, the file is compressed.
ERROR()calls
std::exit, so thetry/catcharound the-bconstructor never ran and there was nosecond chance to improve the message.
This blocks #47 outright, since the GIAB stratification sets ship every region file as
.bed.gz.Change
hts_open()+hts_getline()on akstring_treplace theifstream. htslib decodes plain,gzip, and bgzip transparently and detects the encoding from the file's leading bytes rather than
its extension, so a compressed BED misnamed
.bedis read correctly too. htslib was already alink dependency —
bcf_open()reads the VCFs.The malformed-coordinate
ERRORpaths added under #200 are untouched: the field, the 1-basedline, and the filename are still reported, and now identically whether the file was compressed.
Two incidental consequences of the reader change, both tested:
hts_getlinereturns-1at EOF and<= -2on error; only-1ends the loop. A truncated or corrupt compressed BED previouslywould have parsed as a partial region set and scored against it — it now errors naming the line.
hts_getlinestrips\r\n, wherestd::getlineleftthe
\ron the stop coordinate and rejected the file.The
-busage line now readsBED file containing regions to evaluate (plain, gzip, or bgzip).Testing
Nine
BedCtorcases added alongside the existing ones, none replaced:ParsesUncompressed/ParsesGzip/ParsesBgzipDetectsEncodingFromContents.bedstill parsesMalformedCoordInGzipNamesLineTruncatedGzipErrorsAcceptsCarriageReturnschr1\t2\t8\ryields stop 8, not"8\r"AcceptsEmptyFileOpenFailErrors(existing)write_tmp_bed()gained abedzip_tencoding argument (defaulting to plain, so existing callersare unchanged) that writes the compressed variants via
bgzf_open'swg/wmodes.One integration case,
test_bed_gzipped-regions-match-plain, runs theswallowed_snpscallsetsagainst a gzip copy of
synthetic.bedcompressed into the working directory — no compressedfixture is committed — and pins the same counts as the uncompressed run.
Build fix the tests forced
The second commit adds
-I$(SRC)toTEST_CXXFLAGSintests/unit/build/Makefile.Writing the compressed fixtures needs
"htslib/bgzf.h"fromtests/unit/src/test_helpers.cpp. Aquoted include is resolved against the including file's own directory before any
-I, so thatreached CI's headers only from a file in
src/— where CI copies them, rather than installingthem. It compiled locally regardless, because Homebrew's htslib is on the default search path, so
the first CI run failed with
fatal error: htslib/bgzf.h: No such file or directoryeventhough every local check was green.
Both resolution paths were then verified explicitly: with the headers copied to
src/htslib(theCI layout)
-Hshowsbgzf.hresolving from../../../src/htslib/bgzf.h, and with thatdirectory absent it falls back to the system path.
CXXFLAGSis deliberately untouched, since itmust stay identical to
src/Makefile.Verification
pytest(integration + unit workflow): 115 passed, up from 111.-Wall -Wextra -Werror=missing-field-initializers;doxygen src/Doxyfileemits no warnings.862db48.KS_INITIALIZE,ks_free,hts_getline,bgzf_open'sgmode, andhts_hopen's handling ofempty_formatwere each confirmed present in the 1.17 sources, sincelocal development is against 1.20.
Part of #47; first of nine. Resolves #230 — note this will not auto-close, since the PR targets
devwhile the default branch ismaster.