Repository navigation
Fix 10x support - #12
Merged
Merged
Conversation
The "add 10X support for basic FQ functions" commit registered several
new flags on the wrong command, which panics the entire binary on
startup regardless of which subcommand is invoked:
- cmd/count.go and cmd/extract.go each registered their new --bc-list
flag on convertFqCmd instead of their own Fq command, which also
means convertFqCmd tried to register "bc-list" three times total
(once correctly in convert.go, twice more here) -- pflag panics on
a redefined flag.
- cmd/stats.go registered --bc-list/--threads on unlinkedFqCmd instead
of statsFqCmd, on top of rm_unlinked.go's own (correct) registration
of those same flags on unlinkedFqCmd -- another redefinition panic,
and statsFqCmd ends up with no --bc-list/--threads flags at all.
- cmd/rm_unlinked.go gave unlinkedFqCmd two flags under the same -b
shorthand (--bc-count, a bool from an earlier change, and the new
--bc-list) -- pflag panics on a duplicate shorthand. Its Args func
also still called GetString("bc-count") after that flag became a
bool, which would have been the next failure once the panic was
fixed.
Also added the missing "10x"/"10X" bc-list sentinel exemption to the
Args validators that skip filecheck for a literal whitelist path
(cmd/sort.go, cmd/ncbi.go, cmd/count.go, cmd/stats.go) -- without it,
`--bc-list 10x` (meaning "use the embedded official whitelist") was
rejected before ever reaching fastq.Load10X, which already understood
that sentinel.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015tF7ppXiNo2Adbv8YXcwGL
- cmd/ncbi.go, cmd/sort.go: same missing "10x"/"10X" sentinel
exemption as the previous commit, for the two FASTQ commands whose
--bc-list support predates this feature's addition elsewhere.
- cmd/root.go: fqCmd's Long text still named the flag --bc-map, which
was renamed to --bc-list; fixed the name and a typo ("offical").
- fastq/parser_10x.go: typo in Load10X's doc comment ("rease" the
bcmap file).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015tF7ppXiNo2Adbv8YXcwGL
- stats/stats.fq.go: StatsFQ built a 10X parser/scheme from --bc-list into parse/sch, then unconditionally called "parse, sch, err := chooseParser(fq, platform)" right after -- chooseParser only knows haplotagging/stlfr/tellseq, and reusing parse/sch here (same block as their earlier `var` decls, so this isn't shadowing) discarded the 10X values every time regardless of --bc-list. The entire 10X branch was dead code; `djinn fq stats` always fell through to auto-detection, which 10X data can't satisfy (its barcode is just an unmarked sequence prefix). - invalid/invalid.fq.go: FilterInvalid10XFQ's paired branch never wrote a valid R2 record to R2Writer at all -- the only call to r2core.ToTenX(R2Writer.Writer) was inside the "!r1core.Valid" branch, so prefix.R2.fq.gz came out empty for every properly-paired, valid-barcode 10X input. It also had no separate invalid-output file for 10X (unlike every other format): "kept" invalid records were written into the same file as valid ones instead of their own prefix.invalid.R*.fq.gz. Added R1WriterInv/R2WriterInv and fixed the R2 write to happen unconditionally for the valid case. - unlinked/unlinked.fq.go: RemoveUnlinkedFQ accepted a bcmap parameter (added so rm-unlinked's new --bc-list flag would have somewhere to go) but never used it anywhere in the function body -- 10X support was wired up at the CLI layer and silently absent underneath it. Added RemoveUnlinked10XFQ (and its own getFqCount10X, since a 10X barcode lives only in R1) mirroring the single-end/paired structure already used by FilterInvalid10XFQ/ConvertFrom10X, and dispatch to it from RemoveUnlinkedFQ when bcmap != "". - extract/extract.fq.go: Extract10XFQ duplicated the whitelist-sentinel switch that fastq.Load10X already implements (and that this same "add 10X support" commit introduced), instead of calling it like Count10XFQ/ConvertFrom10X/FilterInvalid10XFQ/StatsFQ all do. No functional difference, just inconsistent with its own siblings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015tF7ppXiNo2Adbv8YXcwGL
CountFQ, ExtractFQ, FilterInvalidFQ, StatsFQ, and RemoveUnlinkedFQ all gained a new parameter in the "add 10X support" commit; every existing call site in these five packages' test files was still using the old signature, so none of them even compiled. None of count/, extract/, invalid/, or unlinked/ had any 10X test coverage at all before this, which is exactly how the bugs fixed in the previous commit went unnoticed. Added: - count: Count10XFQ end-to-end (dedup + unrecognized-barcode drop), plus the "10x" embedded-whitelist sentinel. - extract: Extract10XFQ end-to-end, plus confirms a second (R2) file is correctly ignored rather than misread. - invalid: regression tests for both FilterInvalid10XFQ bugs just fixed (an empty R2 output, and invalid records leaking into the main output instead of their own file), plus single-end. - unlinked: RemoveUnlinked10XFQ singleton/linked routing (paired and single-end) -- this function didn't exist before this branch. - stats: a regression test for the dead-10X-branch bug just fixed, asserting real Barcodes_Valid output rather than just "no error". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015tF7ppXiNo2Adbv8YXcwGL
convert.mdx, sort.mdx, and ncbi.mdx still named the flag --bc-map, which was renamed to --bc-list; fixed throughout, and documented the "10x"/"10X" embedded-whitelist sentinel (fastq.Load10X) that none of the docs mentioned yet. count.mdx, extract.mdx, rm-invalid.mdx, and rm-unlinked.mdx gained real 10X support in the same commit as this docs update but had no mention of it at all -- added a --bc-list row to each options table and a short paragraph explaining why 10X needs it (no self-describing marker, so it can't be auto-detected like haplotagging/stlfr/tellseq) and, for count/extract, that only the first (R1) file is read. rm-unlinked.mdx's options table also still described --bc-count as a string flag with the (now reassigned) -b shorthand; it's a bool that writes to PREFIX.count, documented accordingly. Note: `djinn fq stats` still has no docs page at all (10X-related or otherwise) -- out of scope here since it's not a 10X-specific gap, but worth a follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015tF7ppXiNo2Adbv8YXcwGL
Both `djinn fq stats` and `djinn sam stats` had no documentation page at all. Added one for each, matching the existing pages' format, tone, and length: a short intro, a Usage section with an example, an Options table, and (new here, since neither existing page needed it) an Output table describing the columns each command actually prints. fastq/stats.mdx also documents the --bc-list flag and the 10X embedded-whitelist sentinel, consistent with the other FASTQ docs pages updated for 10X support. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015tF7ppXiNo2Adbv8YXcwGL
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.
No description provided.