Optimize Mutable.nextPermutation and add {next/prev}permutation(By) #498
Conversation
|
Thanks! I'll look into this tomorrow but at a glance |
|
I just aligned it with the original implementation; I assumed it was intentional. Is it better to add it before your review? |
I see. I missed that somehow.
No need. I'll benchmark it and going to tweak code a bit anyway |
|
Yes. This is an optimization. I used very simple benchmark permute :: Int -> Int -> IO (Vector Int)
permute sz n_it = do
vec <- MV.generate sz id
replicateM_ n_it $ MV.nextPermutation vec
V.unsafeFreeze vecWith vector size 20 and BenchmarksI've run five benchmarks:
So for unboxed vector of ints Do you plan to include functions proposed in #499 in this PR? |
I was not sure if it was appropriate to put optimization and API addition in one PR, but I'm happy to do it if you propose so! Should |
Clarified that `Data.Vector.*.Mutable.nextPermutation` does not update the vector when the original state is the last permutation.
This implements some optimization of `nextPermutation` from `Data.Vector.Generic.Mutable`. The main content of this re-implementation is the following two points: 1. Wrapping the whole implementation in `stToPrim`. This allows the compiler to optimize the code better. 2. When finding the rightmost increasing pair v[k]<v[k+1], we now search from the right, instead of from the left. This allows us to abort the search as soon as we find such a pair, giving average-case constant performance, instead of best-case linear in the previous implementation.
This adds the following three companions to the already existing `Data.Vector.*.Mutable.nextPermutation`: - `Data.Vector.*.Mutable.nextPermutationBy` - `Data.Vector.*.Mutable.prevPermutation` - `Data.Vector.*.Mutable.prevPermutationBy` They are all implemented in terms of the already existing `Data.Vector.Generic.Mutable.nextPermutationLt`.
|
Since I forgot to tweak the comments in |
The function `Data.Vector.Generic.Mutable.nextPermutationByLt` is
the unified implementation for the family of functions
`Data.Vector.*.Mutable.{next,prev}Permutation{,By}`.
By adding INLINE pragma to it, we may expect the some performance gain
from specialization.
gksato
force-pushed
the
optimize-nextperm
branch
from
272fa34
to
063c8ef
Compare
|
I added |
gksato
changed the title
|
Oh, and I added: Data.Vector.*.Mutable.nextPermutationBy :: Constraint => (e -> e -> Ordering) -> v (PrimState m) a -> m Bool
Data.Vector.*.Mutable.prevPermutation :: (Constraint, Ord e) => v (PrimState m) a -> m Bool
Data.Vector.*.Mutable.prevPermutationBy :: Constraint => (e -> e -> Ordering) -> v (PrimState m) a -> m BoolThe TODOs we have now are... tests, benchmarks, and changelog? Are they all? |
This adds the following three tests for the pair of functions
`Data.Vector.*.Mutable.{next/prev}Permutation` in
`vector/tests/Tests/Move.hs`:
1. `testRevPermutations`: For n=1,..,7, repeatedly applying
`prevPermutation` to a vector `[n,n-1..1]` produces all n! permutations
of the vector in reverse order, and applying the function to the
lexicographically smallest permutation doesn't change the vector.
2. `testNPPermutationsIsId`: Applying `nextPermutation` followed by
`prevPermutation` to a vector produces the original vector.
Note that this function uses modified versions of `nextPermutation`
and `prevPermutation` that reverse the vector if the original function
returns `False`, rendering those two functions bijective.
3. `testPNPermutationsIsId`: Applying `prevPermutation` followed by
`nextPermutation` to a vector produces the original vector. The same
caveat as above applies here.
|
I've written some tests on a separate branch (gksato:optimize-nextperm-tidyup), since I don't know if it is wanted. I'd fast-forward the branch (gksato:optimize-nextperm) of this pull request to include that commit whenever any of you request it! |
|
I finally found time to review PR. Please add tests and this PR is good to go. It would be nice to add changelog entry as well and whether you want to add benchmarks or not is up to you. |
|
@Shimuuar OK, I'll do that soon. I've been writing some benchmark, so I might add it if implementation doesn't take much time. For the purpose of the changelog and |
|
Yes |
This adds a changelog entry and `@since` annotations for: - Optimization of `Data.Vector.Generic.Mutable.nextPermutation` - Addition of `Data.Vector.Generic.Mutable.prevPermutation(By)` - Addition of `Data.Vector.Generic.Mutable.nextPermutationBy` This also tweaks the haddock comments of the functions for `Data.Vector.*.Mutable.(next|prev)Permutation(By)?` for a better readability.
Implement benchmarks to test performance of nextPermutation and prevPermutation on mutable vectors. Tests include: - Looping through all permutations on small vectors - Applying bijective versions n times on: - Ascending permutations of size n - Descending permutations of size n - Random permutations of size n - For a baseline, copying a vector of size n once. Benchmarks for bijective permutations begins with such a copy, and you might want to remove the impact of copying from the results. Benchmarks cover both forward (next) and reverse (prev) operations.
gksato
force-pushed
the
optimize-nextperm
branch
from
8232899
to
7415257
Compare
|
Added tests, a changelog entry, Note that |
|
That's quite formidable battery of tests all right! It's more thorough than rest of benchmark suite :) I'm unhappy with inlining of worker function. It's too large. But we're running into GHC's limitations since we don't way to ask GHC to generate specializations for it and its callers automatically. Excellent pull request. Thank you!
Yes P.S. |
|
I appreciate your work on review and merge, thank you! Oh, yes. I haven't noticed this would of course be a nice use case for backpack. I imagined what if we try to comply DRY without backpack... urgh, it's a mess!
I truly agree with you. |
This implements some optimization of
nextPermutationfromData.Vector.Generic.Mutable, and supercedes #497. The main content of this re-implementation is the following two points:stToPrim. This allows the compiler to optimize the code better.Also, this adds some clarification on doc comments;
nextPermutationdoes not update the source vector when the given vector is the last permutation.Update
This also adds the following API: