Skip to content

Add permutations with length determined by a const parameter#2

Open
starblue wants to merge 1 commit intomainfrom
feature/const-length
Open

Add permutations with length determined by a const parameter#2
starblue wants to merge 1 commit intomainfrom
feature/const-length

Conversation

@starblue
Copy link
Copy Markdown
Owner

@starblue starblue commented Sep 4, 2022

No description provided.

impl<const N: usize> ConstPermutation<N> {
/// Returns the identity permutation.
pub fn identity() -> Self {
ConstPermutation((0..N).collect::<Vec<_>>().try_into().unwrap())
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Suggested change
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,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't think we need T: Debug. Maybe this was left over from debugging?

Comment on lines +88 to +96
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)
}
Copy link
Copy Markdown

@chrisbouchard chrisbouchard Sep 6, 2022

Choose a reason for hiding this comment

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

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.

Suggested change
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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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);
    }
}

Comment on lines +150 to +154
let mut map = [0; N];
for i in 0..N {
map[i] = other.apply(self.apply(i));
}
ConstPermutation(map)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this just permute?

Suggested change
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))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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<_>>();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this could just be

Suggested change
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) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since this is an unsigned value, I think we can just check

Suggested change
if (0..n).contains(&e) {
if e < n {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Though see my other suggestion about early return.

Comment on lines +10 to +12
if (0..n).contains(&e) {
seen[e] = true;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actually, since this can't be a valid permutation if it contains an out-of-bounds index, we could return immediately.

Suggested change
if (0..n).contains(&e) {
seen[e] = true;
}
if e >= n {
return false;
}
seen[e] = true;

Comment on lines +1 to +3
use core::fmt::Debug;

use std::ops;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should these uses be joined into a single group?

Comment on lines +38 to +52
ConstPermutation(
(0..N)
.map(|k| {
if k == i {
j
} else if k == j {
i
} else {
k
}
})
.collect::<Vec<_>>()
.try_into()
.unwrap(),
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we could reuse identity to make this

Suggested change
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants