Add permutations with length determined by a const parameter #2
Conversation
| impl<const N: usize> ConstPermutation<N> { | ||
| /// Returns the identity permutation. | ||
| pub fn identity() -> Self { | ||
| ConstPermutation((0..N).collect::<Vec<_>>().try_into().unwrap()) |
There was a problem hiding this comment.
It would be great if the const versions of these methods could avoid heap allocations and just allocate the array. E.g., here we could do something like
| ConstPermutation((0..N).collect::<Vec<_>>().try_into().unwrap()) | |
| let mut storage = [0; N]; | |
| for i in 0..N { | |
| storage[i] = i; | |
| } | |
| Self(storage) |
| /// Returns an array permuted by this permutation. | ||
| pub fn permute<T>(&self, a: &[T; N]) -> [T; N] | ||
| where | ||
| T: Clone + Debug, |
There was a problem hiding this comment.
I don't think we need T: Debug. Maybe this was left over from debugging?
| if exp == 0 { | ||
| ConstPermutation::identity() | ||
| } else if exp == 1 { | ||
| self.clone() | ||
| } else if exp % 2 == 0 { | ||
| self.square().pow(exp / 2) | ||
| } else { | ||
| self * self.pow(exp - 1) | ||
| } |
There was a problem hiding this comment.
What would you think of implementing MulAssign for ConstPermutation? Then I think this could be written iteratively using exponentiation by squaring to avoid creating multiple intermediate arrays — which, since they're stored on the stack, could result in a stack overflow much more quickly than with Box<[usize]> in Permutation.
| if exp == 0 { | |
| ConstPermutation::identity() | |
| } else if exp == 1 { | |
| self.clone() | |
| } else if exp % 2 == 0 { | |
| self.square().pow(exp / 2) | |
| } else { | |
| self * self.pow(exp - 1) | |
| } | |
| let mut result = Self::identity(); | |
| let mut term = self.clone(); | |
| while exp > 0 { | |
| if exp % 2 == 1 { | |
| result *= term; | |
| } | |
| term *= term; | |
| exp /= 2; | |
| } | |
| return result; |
There was a problem hiding this comment.
And for what it's worth, MulAssign could be implemented space-efficiently using permute_in_place from #3.
impl<const N: usize> ops::MulAssign<ConstPermutation<N>> for ConstPermutation<N> {
fn mul_assign(&mut self, rhs: ConstPermutation<N>) {
rhs.permute_in_place(&mut self.0);
}
}| let mut map = [0; N]; | ||
| for i in 0..N { | ||
| map[i] = other.apply(self.apply(i)); | ||
| } | ||
| ConstPermutation(map) |
There was a problem hiding this comment.
Is this just permute?
| let mut map = [0; N]; | |
| for i in 0..N { | |
| map[i] = other.apply(self.apply(i)); | |
| } | |
| ConstPermutation(map) | |
| ConstPermutation(other.permute(&self.0)) |
| type Error = TryFromError; | ||
| fn try_from(a: &[usize; N]) -> Result<ConstPermutation<N>, TryFromError> { | ||
| if is_permutation(a) { | ||
| Ok(ConstPermutation(*a)) |
There was a problem hiding this comment.
Huh, I actually had no idea that [T; N]: Copy when T: Copy.
| /// That is, all the elements in `0..len` occur exactly once in the slice. | ||
| pub fn is_permutation(v: &[usize]) -> bool { | ||
| let n = v.len(); | ||
| let mut seen = (0..n).map(|_| false).collect::<Vec<_>>(); |
There was a problem hiding this comment.
I think this could just be
| let mut seen = (0..n).map(|_| false).collect::<Vec<_>>(); | |
| let mut seen = vec![false; n]; |
| let n = v.len(); | ||
| let mut seen = (0..n).map(|_| false).collect::<Vec<_>>(); | ||
| for &e in v { | ||
| if (0..n).contains(&e) { |
There was a problem hiding this comment.
Since this is an unsigned value, I think we can just check
| if (0..n).contains(&e) { | |
| if e < n { |
There was a problem hiding this comment.
Though see my other suggestion about early return.
| if (0..n).contains(&e) { | ||
| seen[e] = true; | ||
| } |
There was a problem hiding this comment.
Actually, since this can't be a valid permutation if it contains an out-of-bounds index, we could return immediately.
| if (0..n).contains(&e) { | |
| seen[e] = true; | |
| } | |
| if e >= n { | |
| return false; | |
| } | |
| seen[e] = true; |
| use core::fmt::Debug; | ||
|
|
||
| use std::ops; |
There was a problem hiding this comment.
Should these uses be joined into a single group?
| ConstPermutation( | ||
| (0..N) | ||
| .map(|k| { | ||
| if k == i { | ||
| j | ||
| } else if k == j { | ||
| i | ||
| } else { | ||
| k | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .try_into() | ||
| .unwrap(), | ||
| ) |
There was a problem hiding this comment.
I think we could reuse identity to make this
| ConstPermutation( | |
| (0..N) | |
| .map(|k| { | |
| if k == i { | |
| j | |
| } else if k == j { | |
| i | |
| } else { | |
| k | |
| } | |
| }) | |
| .collect::<Vec<_>>() | |
| .try_into() | |
| .unwrap(), | |
| ) | |
| let mut result = Self::identity(); | |
| result.0.swap(i, j); | |
| result |
No description provided.