Added `array_permutations` (attempt 2) #1014

ronnodas · 2025-01-13T12:29:54Z

Most of the changes are to create a common abstraction that works for permutations and {array_,}combinations, much of it by @phimuemue. Completes a variant of #1001.

codecov · 2025-01-13T12:31:43Z

Codecov Report

Attention: Patch coverage is 94.17476% with 6 lines in your changes missing coverage. Please review.

Project coverage is 94.38%. Comparing base (6814180) to head (1348785).
Report is 134 commits behind head on master.

Files with missing lines	Patch %	Lines
src/lazy_buffer.rs	89.47%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1014      +/-   ##
==========================================
- Coverage   94.38%   94.38%   -0.01%     
==========================================
  Files          48       50       +2     
  Lines        6665     6930     +265     
==========================================
+ Hits         6291     6541     +250     
- Misses        374      389      +15

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

phimuemue

Importing things: Do we use use something as _ in our codebase? If not, please go with use something;.
Is ArrayOrVecHelper for Vec<usize> even needed after all this? Shouldn't combinations also run via Box<[usize]>?

src/combinations.rs

phimuemue · 2025-01-13T14:18:31Z

src/combinations.rs

 use std::fmt;
 use std::iter::FusedIterator;

-use super::lazy_buffer::LazyBuffer;
+use super::lazy_buffer::{LazyBuffer, PoolIndex};


I think PoolIndex is not really tied to LazyBuffer (it should be compatible with anything you can usize-index into), so it should probably have an own module.

phimuemue · 2025-01-13T14:24:24Z

src/permutations.rs

-        cycles: Box<[usize]>, // TODO Should be Idx::Item<usize>
-        k: Idx::Length,       // TODO Should be inferred from cycles
-    },
+    Loaded { indices: Box<[usize]>, cycles: Idx },


Is cycles really an Idx or should it actually be Idx::Item<usize>? I know that these are the same types, but don't the semantics differ? I.e. isn't Idx always meant to index into a slice, whereas Idx::Item<usize> is an array or a Vec containing usizes?

If we'd like to avoid answering this question, we could keep Loaded::k: Idx::Length.

cycles is really neither an Idx nor an Idx::Item<usize>, since Idx::Item should be what is obtained by indexing something of type Idx into the buffer, but I don't feel strongly about this. Maybe it makes sense to keep cycles of type Box<[usize]> and switch back to the name PoolIndex so that that trait has clearer semantics.

src/permutations.rs

phimuemue · 2025-01-13T14:29:17Z

src/permutations.rs

-                        cycles,
-                        k: *k,
-                    };
+                    let item = Idx::item_from_fn(k, |i| vals[indices[i]].clone());


Isn't this indices.extract_item(vals)?

No, for the same reason as in the PermutationState::Loaded case, indices is now of length n.

phimuemue · 2025-01-13T14:33:49Z

src/permutations.rs

@@ -173,22 +165,26 @@ impl<Idx: ArrayOrVecHelper> PermutationState<Idx> {
            let total = (n - k.value() + 1..=n).try_fold(1usize, |acc, i| acc.checked_mul(i));
            (total.unwrap_or(usize::MAX), total)
        };
-        match *self {
+        match self {


Why did this change from *self to self? Is this really realted to this commit?

We can take this, but I prefer not to mix it into these commits, if possible.

It is related: Buffered.indices is no longer Copy, we could add a ref there instead.

Or alternatively also remove the refs in the Loaded case.

src/combinations.rs

phimuemue · 2025-01-13T14:40:59Z

src/combinations.rs

@@ -57,7 +57,7 @@ where
    debug_fmt_fields!(Combinations, indices, pool, first);
 }

-impl<I: Iterator, Idx: PoolIndex> CombinationsGeneric<I, Idx> {
+impl<I: Iterator, Idx: ArrayOrVecHelper> CombinationsGeneric<I, Idx> {


Is Idx still a good name if the trait is ArrayOrVecHelper?

ronnodas · 2025-01-13T15:16:03Z

Is ArrayOrVecHelper for Vec even needed after all this? Shouldn't combinations also run via Box<[usize]>?

This is needed to allow Combinations::reset, which is (only) used by powerset.

phimuemue · 2025-01-15T09:50:56Z

@jswrenn I'd like to bump to Rust 1.65, because it allows generic associated types.

We currently have trait PoolIndex<T>, but what we actually want is trait ContiguousSequence { type Type<T>: BorrowMut<[T]>; } which could then be impld for Vecs and arrays. Then, we could use ContiguousSequence::Type with different Ts (nice, because the indices/cycles use ContiguousSequence::Type<usize> and e.g. Permutations::Item = ContiguousSequence::Type<UnderlyingIterator::Item>).

I tried staying in Rust 1.63, but we'd have to bend over backwards to achieve something similar.

Please let me know if I can do this, then I'll prepare a PR changing PoolIndex<T> to ContiguousSequence, paving the way for array_permutations.

@ronnodas I'll keep you in the loop.

phimuemue and others added 11 commits January 9, 2025 18:07

Treat k==0 less special

e27b3ee

Treat k==0 less special (2)

8ae44e0

Introduce MaybeConstSize

9b3d6e2

PoolIndex<T> -> PoolIndex

5003927

MaybeConstSize (2)

5826470

Express some things via PoolIndex/MaybeConstUsize

aa90cbb

Try to use PoolIndex for permutations

98b79de

Move MaybeConstUsize, PoolIndex to lazy_buffer

79f52b2

Rename PoolIndex to ArrayOrVecHelper

85a84bd

Rename ArrayOrVecHelper::from_fn to item_from_fn

6056b73

Use Idx type directly in PermutationState

99de003

ronnodas mentioned this pull request Jan 13, 2025

add array_permutations() #1013

Closed

ronnodas added 3 commits January 13, 2025 14:05

Use Box<[usize]> instead of Vec<usize> in Permutations

d8de4b8

Use ArrayOrVecHelper::start in {Array,}Combinations

57fff6e

Update ArrayOrVecHelper doc comments

0b96072

ronnodas force-pushed the array-permutations branch from a56b5c7 to 0b96072 Compare January 13, 2025 13:05

phimuemue requested changes Jan 13, 2025

View reviewed changes

ronnodas added 2 commits January 13, 2025 16:19

Avoid use ... as _

406eac2

Specify generic in ArrayCombinations::new

1348785

ronnodas added 2 commits January 16, 2025 15:16

Add debug_assert in advancing PermutationState::Buffered

3dd45f5

Use last_mut()? instead of indexing

5a7c522

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `array_permutations` (attempt 2) #1014

Added `array_permutations` (attempt 2) #1014

ronnodas commented Jan 13, 2025

codecov bot commented Jan 13, 2025 •

edited

Loading

phimuemue left a comment

phimuemue Jan 13, 2025 •

edited

Loading

phimuemue Jan 13, 2025

ronnodas Jan 13, 2025 •

edited

Loading

phimuemue Jan 13, 2025

ronnodas Jan 13, 2025

phimuemue Jan 13, 2025

ronnodas Jan 13, 2025 •

edited

Loading

phimuemue Jan 13, 2025

ronnodas commented Jan 13, 2025

phimuemue commented Jan 15, 2025 •

edited

Loading

Added array_permutations (attempt 2) #1014

Are you sure you want to change the base?

Added array_permutations (attempt 2) #1014

Conversation

ronnodas commented Jan 13, 2025

codecov bot commented Jan 13, 2025 • edited Loading

Codecov Report

phimuemue left a comment

Choose a reason for hiding this comment

phimuemue Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

phimuemue Jan 13, 2025

Choose a reason for hiding this comment

ronnodas Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

phimuemue Jan 13, 2025

Choose a reason for hiding this comment

ronnodas Jan 13, 2025

Choose a reason for hiding this comment

phimuemue Jan 13, 2025

Choose a reason for hiding this comment

ronnodas Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

phimuemue Jan 13, 2025

Choose a reason for hiding this comment

ronnodas commented Jan 13, 2025

phimuemue commented Jan 15, 2025 • edited Loading

Added `array_permutations` (attempt 2) #1014

Added `array_permutations` (attempt 2) #1014

codecov bot commented Jan 13, 2025 •

edited

Loading

phimuemue Jan 13, 2025 •

edited

Loading

ronnodas Jan 13, 2025 •

edited

Loading

ronnodas Jan 13, 2025 •

edited

Loading

phimuemue commented Jan 15, 2025 •

edited

Loading