Skip to content
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

RFC 0005: Combinatoric library #5

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

meatball133
Copy link

Add methods for permutations and combinations for Int.

Comment on lines 109 to 110
Since both of these methods quickly reach large numbers so do they both have to be implemented under `BigInt`.
They could either be written manualy using the mathematical formula togehter with `BigInt#factorial` or using a more optimzied version.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GMP already defines mpz_bin_ui for BigInt#combinations(LibGMP::UI). I am not sure whether #permutations exists.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed a couple of spelling and grammar mistakes. That's not too bad, but it noticable hinders the reading effort. Could you perhaps try running a spell checker over the text?
Much appreciated 🙏

Crystal has an already exsisting methods for doing combinations and permutations but those work on some form of collection which for larger sizes makes them very computanional heavy and high memory usage.
In the scientific world is there many uses of getting to know the size of combinations or permutations but not the individual combinations or permutations.
These methods could also have an `Int` implementation which doesnt use any form of collections to reduce on the calculations and memory usage required.
There are already exsisting libraries which impliments this logic but most of them are unmaintained.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a few references to those libraries? Do some of those already have an optimized implementation?


Crystal has an already exsisting methods for doing combinations and permutations but those work on some form of collection which for larger sizes makes them very computanional heavy and high memory usage.
In the scientific world is there many uses of getting to know the size of combinations or permutations but not the individual combinations or permutations.
These methods could also have an `Int` implementation which doesnt use any form of collections to reduce on the calculations and memory usage required.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anything, the Int::Primitive definitions should come first before the BigInt one, e.g. using the multiplicative definition. Having the methods available only in BigInt greatly restricts its utility.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but combinatoric calculations work on very big numbers. I don't know if a more efficient method can be found. but the one which is defined in math. For example 20.combinations(5). Would do 20!, which is already 2.432902e+18. And then divide that number, you could likely optimize it a bit, but it is still big numbers which are being worked on. So aslong as the group is small it is fine but when it passes like 50 for even an optimized version so do it require bigint.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good start is something like 20 // 1 * 19 // 2 * 18 // 3 * 17 // 4 * 16 // 5. The divisions are all exact, because the first n multiplied values must have exactly one multiple of n itself, and this will minimize the possibility of intermediate overflows. It makes sense that 50.combinations(2) would simply do 50 * 49 // 2 without ever calling any factorial function.

There are already exsisting libraries which impliments this logic but most of them are unmaintained.

The expected outcome out of this is that Crystal will have a stronger standard library for working with scientific purposes.
But also when working with just sizes of permutations and combinations will have drasticly faster executions.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One particularly important use case is preallocating the array used for Indexable#each_combination:

# note: this should not require `BigInt` to work!
ary = Array(Array(Int32)).new(6.combinations(3) + 1)
[0, 1, 4, 9, 16, 25].each_combination(3) { |v| arr << v }
arr << 0 # okay, no reallocation

Whether Indexable#combinations should also use the same preallocation is a different question. The standard library is currently not quite consistent in that regard. (Contrast with crystal-lang/crystal#10075)

Then the order matters you put them in the list.

Permutations doesnt have the same pattern as combinations with `5.combinations(3)` is the same as `5.combinations(2)`.
Instead for permutations so are `n.combinations(n)` always equal `n.combinations(n - 1)` (as long as n is a posstive number and not zero).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo?

Suggested change
Instead for permutations so are `n.combinations(n)` always equal `n.combinations(n - 1)` (as long as n is a posstive number and not zero).
Instead for permutations so are `n.permutations(n)` always equal `n.permutations(n - 1)` (as long as n is a posstive number and not zero).

Comment on lines 97 to 101
number.combinations(5)
# => 120

number.combinations(4)
# => 120

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
number.combinations(5)
# => 120
number.combinations(4)
# => 120
number.permutations(5)
# => 120
number.permutations(4)
# => 120

```crystal
require "big"

number : BigInt = 5

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These TypeDeclarations will not compile. They should be BigInt.new(5) or 5.to_big_i

Comment on lines 70 to 72
number2 : BigInt = -5
number2.combinations(2)
# ArgummentError: combinations can't be done on negativ integers
Copy link

@HertzDevil HertzDevil Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#combinations can be generalized to negative ns by letting (-n).combinations(k) == (n + k - 1).combinations(k) * (-1) ** k.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well you are sorta never dealing with negative combinations, and this method in Python and my calculator returns error when trying to use a negative integer. Then also the combination method is in math derived from factorial and in Crystal does the factorial method not accept negative integers.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you said that these methods contribute to "a stronger standard library for working with scientific purposes", and mathematical science dictates that this generalization is valid, otherwise there wouldn't be a reason GMP explicitly states that negative n is supported (original Knuth paper).

IMO the restriction to non-negative n here (even k) is too artificial.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, I am perhaps not the best at math, but I find the problem with negative numbers is that most of this works around vectors. Vectors' size can't be negative, but their direction can be. This means if we have the vector : {3, 4} and would multiply it by -1, then it would be {-3, -4}. Its direction is different, but its length or size remains the same, which would be: (-3 ^ 2 + -5 ^2) ^ 0.5 = 5. If we start allowing negative integers, then we start claiming that these vectors' size is negative, which is mathematically impossible without using complex numbers.

With this said, I have a hard time seeing how this can be generalized with negative numbers. I barely know anything about GMP, and I am up for changes if it is possible to generalize these methods, but I am not aware of how that can be implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants