Fix BitSet copies above 256 bits; compute MRegCCD boundary counts by indexing - #19
Open
alexeid wants to merge 2 commits into
Open
Fix BitSet copies above 256 bits; compute MRegCCD boundary counts by indexing#19alexeid wants to merge 2 commits into
alexeid wants to merge 2 commits into
Conversation
newBitSet(BitSet) sized the copy with other.length() -- the index of the highest set bit plus one -- where it needed other.size(), the capacity. A set whose top words happen to be empty therefore came back with a shorter word array. That is not only a crash. and/or/xor/andNot/intersects all iterate over this.words.length and index the operand directly, so an undersized operand throws ArrayIndexOutOfBoundsException while an undersized *receiver* silently drops the high words instead. It bites only above 256 bits, where the generic BitSet replaces the fixed-size subclasses, and only when the top words are empty -- the normal case for a clade that does not contain the highest-numbered taxa. It surfaced as a crash in MRegCCD on a 276-taxon analysis and cannot affect analyses of 256 taxa or fewer. BitSetCopyTest covers 276/320/512/1000 bits and the empty set, exercising both directions of andNot and intersects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MRegCCD found its boundaries by walking every ordered choice of parts, costing O(m^(k-1)) in the number m of observed subclades of a clade, and guarded that with a flat 20M op budget. On large analyses the budget bit before the enumeration finished, and countsFor then caught the overflow and left the remaining orders at zero -- which also silently disabled the tail correction, since tailFor reads the top two orders. Nothing was reported to the caller. Measured on RSV2 (129 taxa) the shipped defaults returned -60.6219 where the untruncated answer is -60.6443. The counts are now found by indexing the disjoint pairs of observed subclades by the bitset they cover, once per clade in O(m^2): a boundary of 2 is a subclade whose complement is observed, of 3 a subclade whose complement is covered by a pair, of 4 a pair whose complement is covered by another pair. This is how KRegCCD computes its own reserve, which counts the same partitions indexed by novel-clade count rather than boundary size. Orders beyond 4, and depths below 4, defer to the direct enumeration, which is cheaper there. The former implementation is kept as MRegCCDSlow, and MRegCCD extends it overriding countsFor alone, so the model is defined in exactly one place and the two can be checked against each other. MRegCCDAgreementTest confirms identical boundary counts on 2199 clades and identical tree probabilities across 6/8/10/14 taxa and depths 2 to 4, and checks the exact-normalisation guarantee directly on the class callers get. Both now default to reserve depth 4, matching KRegCCD's default reserve (k = 2, boundaries 3 and 4), so the two models look equally far past the CCD graph. The previous default of 5 went a boundary further than KRegCCD while the op budget stopped it from ever getting there. On RSV2 at the new default this is 4.3x faster than the direct enumeration (1034ms against 4396ms to score 200 trees) and returns the untruncated value. MRegCCDParameterOptimiser needs no change: it names MRegCCD, which is now the indexed implementation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Two independent fixes, one commit each, branched from
master.1. Undersized
BitSetcopies above 256 bitsnewBitSet(BitSet other)sized the copy withother.length()— the index of the highest set bit plus one — where it neededother.size(), the capacity.This is worse than a crash. All five bitwise operations iterate
this.words.lengthand index the operand directly, so an undersized operand throwsArrayIndexOutOfBoundsException, while an undersized receiver silently drops the high words instead.It bites only above 256 bits, where the generic
BitSetreplaces the fixed-size subclasses, and only when the top words are empty — the normal case for a clade not containing the highest-numbered taxa. Analyses of 256 taxa or fewer cannot be affected. It surfaced as a crash in MRegCCD on a 276-taxon dataset.BitSetCopyTestcovers 276/320/512/1000 bits and the empty set.2. MRegCCD boundary counts
The counts were found by walking every ordered choice of parts,
O(m^(k-1))in the numbermof observed subclades, guarded by a flat 20M op budget. On large analyses the budget bit before the enumeration finished, andcountsForthen caught the overflow and left the remaining orders at zero — which also silently disabled the tail correction, sincetailForreads the top two orders. Nothing was reported to the caller.Measured on RSV2 (129 taxa): the shipped defaults returned −60.6219 where the untruncated answer is −60.6443.
The counts are now found by indexing the disjoint pairs of observed subclades by the bitset they cover, once per clade in
O(m^2):This is how
KRegCCDcomputes its own reserve — the same partitions, indexed by novel-clade count rather than boundary size. Orders beyond 4, and depths below 4, defer to the direct enumeration, which is cheaper there.Both now default to reserve depth 4, matching KRegCCD's default (
k = 2, boundaries 3 and 4), so the two models look equally far past the CCD graph. The previous default of 5 went a boundary further than KRegCCD while the budget stopped it from ever getting there.Structure
The former implementation is kept as
MRegCCDSlow, andMRegCCD extends MRegCCDSlowoverridingcountsForalone — so the model is defined in exactly one place and the two are checkable against each other rather than being independent reimplementations.MRegCCDAgreementTestconfirms identical boundary counts on 2199 clades and identical tree probabilities across 6/8/10/14 taxa and depths 2–4, and checks the exact-normalisation guarantee directly on the class callers get — previously only verified on the reference implementation.MRegCCDParameterOptimiserneeds no change: it namesMRegCCD, which is now the indexed implementation. That matters, as it builds a model per CV fold and is the heaviest consumer of these counts.Performance, RSV2, 200 test trees
Caveats
MRegCCDSlowis only slower at boundary 4 and above; below that it is the faster of the two, which is whyMRegCCDdefers to it there. The name describes the default configuration.KRegCCDwas checked for the same problem and does not truncate at its default — identical results at a 100× budget — because its budget is scaled to its algorithm (8m^2) rather than flat.🤖 Generated with Claude Code