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

Const generic permutations #954

Closed
wants to merge 1 commit into from
Closed

Conversation

FlareFlo
Copy link

In an application of mine i use a fixed size of permutations, which would cut down on some very wasteful temporary allocations. Therefore I am proposing a const-generic counterpart to the regular permutations.

This drafts purpose is to get some initial feedback on the idea.
Do not expect feature completeness or sanity, it is 3AM. Though I do intend to properly implement this if interest exists.

Comment on lines +70 to +74
PermutationState::Start => {
*state = PermutationState::End;
// TODO: Consider this case and handle it somehow, currently just using default
Some(std::array::from_fn(|_|I::Item::default()))
}
Copy link
Author

Choose a reason for hiding this comment

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

Not sure how to proceed about this, from what i can tell the API guarantees the vector to always be K elements long, but the reference returns an empty vec?

Comment on lines +104 to +114
let item = vals.get_at(&indices[0..K]); // TODO: Impl const sized variant otherwise this is pointless
*state = PermutationState::Loaded { indices, cycles };
Some(item.try_into().ok()?) // TODO: Handle error case
}
}
PermutationState::Loaded { indices, cycles } => {
if advance(indices, cycles) {
*state = PermutationState::End;
return None;
}
let k = cycles.len();
Copy link
Author

Choose a reason for hiding this comment

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

Indexing into LazyBuffer would require a const-generic function also, otherwise this effort would be pointless

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented May 31, 2024

Unless the lexicographic order of the permutations is important for you, the permutohedron crate should plainly satify you (created by bluss who first wrote itertools). It should even be faster than a const-generics version of our permutations, due to the algorithm.

Now, some context... Right now, our MSRV is 1.43.1 while const-generics require 1.51.0. We will move to it in a near future though. But core::array::from_fn requires 1.63.0 and core::array::try_from_fn is still nightly, same for Iterator::next_chunk. So we are missing some helpful bricks at the moment.

This will eventually be done along with combinations & Co. but not immediately. #560 would be a more intermediary step.

@Philippe-Cholet Philippe-Cholet added the const-generics Require Rust 1.51 or newer label May 31, 2024
Copy link

codecov bot commented May 31, 2024

Codecov Report

Attention: Patch coverage is 0% with 97 lines in your changes are missing coverage. Please review.

Project coverage is 93.23%. Comparing base (6814180) to head (80fc48d).
Report is 99 commits behind head on master.

Files Patch % Lines
src/permutations_const.rs 0.00% 90 Missing ⚠️
src/lib.rs 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #954      +/-   ##
==========================================
- Coverage   94.38%   93.23%   -1.16%     
==========================================
  Files          48       50       +2     
  Lines        6665     7143     +478     
==========================================
+ Hits         6291     6660     +369     
- Misses        374      483     +109     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@FlareFlo
Copy link
Author

Unless the lexicographic order of the permutations is important for you, the permutohedron crate should plainly satify you (created by bluss who first wrote itertools). It should even be faster than a const-generics version of our permutations, due to the algorithm.

Now, some context... Right now, our MSRV is 1.43.1 while const-generics require 1.51.0. We will move to it in a near future though. But core::array::from_fn requires 1.63.0 and core::array::try_from_fn is still nightly, same for Iterator::next_chunk. So we are missing some helpful bricks at the moment.

This will eventually be done along with combinations & Co. but not immediately. #560 would be a more intermediary step.

permutohedron covers my case, thanks!

Seeing that the MSRV is restricted like that, should I close this?

@Philippe-Cholet
Copy link
Member

If you want to revisit this when a more appriopriate time come then we can re-open it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
const-generics Require Rust 1.51 or newer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants