Fix compositions count freqs target - #65
Merged
Merged
Conversation
Pass `!includeZero || isWeak` to `CountPartsMultiset` so zero-exclusion is respected, and add a regression test for sum constraints with frequencies.
Use gpt-5.5 as the default review model across helpers, allow AI_REVIEW_MODEL overrides for news and PR summaries, and omit temperature from response requests.
Extract the zero-counting flag to document how padded zeros are handled in composition multiset counts.
jwood000
commented
Jun 6, 2026
jwood000
left a comment
jwood000
left a comment
Owner
Author
There was a problem hiding this comment.
Reviewed the fix. The change is intentionally narrow: CountPartsMultiset() already has the correct counting behavior once the final flag reflects whether zeros should participate in the permutation count.
The previous call passed part.isWeak directly, which undercounted non-weak composition cases where zero was not part of the input but appeared internally as padding. The updated condition, !part.includeZero || part.isWeak, preserves the existing weak behavior while ensuring fixed-length compositions over positive inputs are counted correctly.
The added regression test covers the reported mismatch between permuteCount() and compositionsCount().
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
This PR fixes a counting bug in
compositionsCount()for multiset inputs with repeated frequencies, a fixed length, and a target constraint when the input does not include zero.Previously, the following equivalent counting paths returned different results:
permuteCount( 1:15, 8, freqs = rep(5:1, each = 3), constraintFun = "sum", comparisonFun = "==", limitConstraints = 40 ) # 12527578 compositionsCount( 1:15, 8, freqs = rep(5:1, each = 3), target = 40 ) # previously 3262902 # now 12527578The fix updates the multiset composition/partition counting path so the weak-counting flag is set correctly when zero is not included, or when the count is explicitly weak.
Changes
temperatureparameters.Notes
No API changes are introduced. Existing code remains source-compatible, but counts for affected cases may change from the previous incorrect value to the corrected value.