Repository navigation
Sync headers from barry 0.2.2: fix FreqTable/vecHasher hash collisions - #8
Merged
Merged
Conversation
…sher Syncs inst/include/barry/ with USCbiostats/barry#24, which fixes a catastrophic hash-collision bug: std::hash of small-integer doubles has no entropy in its low bits, and the boost-style combine never propagated any down, so unordered_map (which masks the hash with a power-of-two bucket count) placed all keys in a single bucket. Support computation degenerated to O(4^ncol) and models with many response columns (e.g., the 16-outcome DEFM that surfaced this) appeared to hang in init. Both hash sites now apply a splitmix64 finalizer per element, and FreqTable::add() verifies the stored row on a hash hit, chaining true 64-bit collisions instead of silently merging distinct statistics. Downstream check: defm rebuilt against these headers passes its full tinytest suite (25 results); a 16-column model init drops from >5 minutes (never observed to finish on real data) to ~1.4s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thank you for your contribution @gvegayon 🚀! The pkgdown site preview is ready for review 👉 Download here 👈! |
…ards Follows USCbiostats/barry#24 review: the splitmix64 finalizer now mixes in an explicit uint64_t (identical avalanche on 32- and 64-bit targets), make_hash/vecHasher guard against empty input, and a length_error typo is fixed. Keeps inst/include/barry byte-identical to barry master. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
gvegayon
added a commit
to UofUEpiBio/defm
that referenced
this pull request
Jul 15, 2026
CI could not resolve `barry (>= 0.2.2)`: CRAN only has 0.2.1, and pak's `deps::.` (used by setup-r-dependencies) does not consume the DESCRIPTION `Remotes:` field when a satisfying-named package exists on CRAN, so the GitHub source for the newer barry was never considered. Add the barryr PR branch as an explicit `extra-packages` entry in every workflow that installs dependencies. The action passes extra-packages straight to `pak::lockfile_create()`, which does honor an explicit remote spec; verified locally with pak 0.10.0 in a clean library that the graph now solves with barry -> 0.2.2 from the branch. All three entries are temporary and must be removed once barry 0.2.2 is on CRAN (tracked via USCbiostats/barryr#8); each is commented as such. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
gvegayon
added a commit
to UofUEpiBio/defm
that referenced
this pull request
Jul 15, 2026
USCbiostats/barryr#8 is merged, so barryr's main branch is barry 0.2.2. Drop the PR-branch ref from the extra-packages remotes; plain USCbiostats/barryr now resolves to 0.2.2 and won't break when the branch is deleted. Verified with pak 0.10.0 in a clean library: the graph solves with barry -> 0.2.2 from barryr main (commit fdd41d5). Still temporary: remove these extra-packages entries entirely once barry 0.2.2 reaches CRAN. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
gvegayon
added a commit
to UofUEpiBio/defm
that referenced
this pull request
Jul 15, 2026
…uptible (#8) * Require barry >= 0.2.2 and make long computations interruptible barry 0.2.2 (USCbiostats/barry#24) fixes a catastrophic hash-collision bug that made init_defm() appear to hang for models with many outcome columns: all support statistics landed in a single unordered_map bucket, degrading support enumeration to O(4^ncol). A 16-outcome, order-1 model that previously never finished initializing now takes seconds. The LinkingTo requirement ensures defm is compiled against fixed headers. This also defines BARRY_USER_INTERRUPT as Rcpp::checkUserInterrupt() in every translation unit that includes barry, enabling barry's built-in interrupt checkpoint (every 1000 support iterations). Long-running calls such as init_defm() can now be stopped with Ctrl-C from an interactive session instead of requiring a force-kill. Verified via setTimeLimit(), which uses the same R_CheckUserInterrupt checkpoint: a ~20s init aborts at the 3s limit with a clean R error and no crash. A model interrupted mid-initialization should be rebuilt before further use (same caveat as any error thrown during init). All 25 tinytest results pass against barry 0.2.2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Adding the remotes field in the description (will remove once the package lands on CRAN) * ci: install barry 0.2.2 from barryr PR branch until it reaches CRAN CI could not resolve `barry (>= 0.2.2)`: CRAN only has 0.2.1, and pak's `deps::.` (used by setup-r-dependencies) does not consume the DESCRIPTION `Remotes:` field when a satisfying-named package exists on CRAN, so the GitHub source for the newer barry was never considered. Add the barryr PR branch as an explicit `extra-packages` entry in every workflow that installs dependencies. The action passes extra-packages straight to `pak::lockfile_create()`, which does honor an explicit remote spec; verified locally with pak 0.10.0 in a clean library that the graph now solves with barry -> 0.2.2 from the branch. All three entries are temporary and must be removed once barry 0.2.2 is on CRAN (tracked via USCbiostats/barryr#8); each is commented as such. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: point barry remote at barryr main now that #8 is merged USCbiostats/barryr#8 is merged, so barryr's main branch is barry 0.2.2. Drop the PR-branch ref from the extra-packages remotes; plain USCbiostats/barryr now resolves to 0.2.2 and won't break when the branch is deleted. Verified with pak 0.10.0 in a clean library: the graph solves with barry -> 0.2.2 from barryr main (commit fdd41d5). Still temporary: remove these extra-packages entries entirely once barry 0.2.2 reaches CRAN. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <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
1 participant
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.
Summary
Syncs
inst/include/barry/with USCbiostats/barry#24 and bumps the package to 0.2.2, followingbarry's versioning.What the upstream fix does:
std::hashof small-integer doubles (0.0, 1.0, ...) is a bit-cast with ~50 zero low bits, and the boost-style combine inFreqTable::make_hash/vecHashernever propagated entropy into the low bits. libc++'sunordered_mapmasks the hash with a power-of-two bucket count — exactly those bits — so all keys landed in a single bucket and support computation degenerated from O(2^ncol) to O(4^ncol). In practice,defm::init_defm()on a 16-outcome model appeared to hang indefinitely (this is how the bug surfaced, via a UofUEpiBio/defm user). Both hash sites now apply a splitmix64 finalizer per element, andFreqTable::add()additionally verifies the stored row on a hash hit so true 64-bit collisions chain instead of silently merging distinct statistics.After the sync, the vendored tree is byte-identical to
barrymaster + PR 24 (diff -rqclean).Measurements (via defm rebuilt against these headers)
init_defm, 16 outcome columns, 1 unique Markov stateinit_defm, 16 outcome columns, 20 unique Markov statesTesting
R CMD INSTALLcleandefmrecompiled against these headers passes its full tinytest suite (25 results)make test-portablepasses (122 assertions, 21 test cases) on the same header setMerge order: ideally after USCbiostats/barry#24, so the vendored headers correspond to a tagged upstream state. A companion PR updates
defmto requirebarry (>= 0.2.2).🤖 Generated with Claude Code