-
Notifications
You must be signed in to change notification settings - Fork 448
All combinations #51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
All combinations #51
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
5997178
Add tests for `Combination`’s `count` property
mdznr a4c5e8c
Document `Combinations`’s `count` property
mdznr cd9d372
Make `Combinations`’s `k` a `let` instead of a `var`
mdznr 88bd1db
Correct function signature in comment
mdznr cdc3d25
Add ability to iterate through `Combinations` of all sizes
mdznr 0465522
Add tests for `Combinations` with a range of accepted sizes
mdznr 8ed61a6
Make `Combinations` iterate in increasing order of size
mdznr 0618c70
Document additions to `Combinations`: `combinations(ofCounts:)` and `…
mdznr 1abd5dd
Add ability to use partial ranges, instead of just `ClosedRange`, for…
mdznr dd2a1fa
Add tests for `Combinations` using partial ranges
mdznr cbf6f37
Simplify range expression by not using `R.Bound` since it’s always `Int`
mdznr ab42094
Increment range’s bound with `+ 1` instead of `advanced(by: 1)`
mdznr 932f357
Rename `k` to `kRange` to indicate that it now represents a range of `k`
mdznr f3dae04
Remove `combinations()` in favor of using `0...`
mdznr a1f6b40
Re-do line wrap in Combinations.md
mdznr 5ec5f6a
Update copyright year for files modified in 2021
mdznr 70ad269
Rename `combinations(ofCounts:)` to `combinations(ofCount:)`
mdznr 4c4100f
Avoid any intermediate heap allocations when advancing `indexes`
mdznr 822b46b
Avoid counting `base` more than once
mdznr dcf4054
Clarifying comment about behavior of limited range
mdznr 7a6c135
Update documentation on complexity of `combinations(ofCount:)`
mdznr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future optimization — the binomial coefficient is symmetric around N/2 or (N - 1)/2, so up to half the work could be skipped here. Or there might be a way to calculate this directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I’ll play around with a way to do this that maintains readability. We can probably get most of the algorithmic performance benefits by using memoization in
binomial
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested adding a small memoization cache (
Dictionary
) tobinomial
. To get thecount
value forCombinations
of aCollection
of length 26 ("ABCDEFGHIJKLMNOPQRSTUVWXYZ"
), using count1..<26
it cut down the number of calls tobinomial
from 206 to 128.I have some ideas on how to further improve performance for this. There is already a special case when the range of
k
is0...base.count
to use1 << n
(2^n). However, a potential common case is excluding the empty and/or complete set (1..<base.count
), which we can easily calculate using 2^n minus 1 (or 2). We can do some work to get the absolute minimum number of computations necessary to calculate a contiguous range of values in the last row of the arithmetic triangle.Should I move that to a separate, subsequent PR so not to block this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That can be done later for sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#58