jack-pappas / ExtCore

An extended core library for F#.
Apache License 2.0
178 stars 32 forks source link

Async.Seq.Parallel.batch wrong implementation? #15

Closed xkrt closed 6 years ago

xkrt commented 9 years ago

Hi, after reading (and rereading a few times) xml doc for ExtCore.Control.Collections.Async.Seq.Parallel.batch I expect:

[1..10]
|> Seq.map (fun x -> async { return x * x })
|> ExtCore.Control.Collections.Async.Seq.Parallel.batch 3

expect to return [1; 4; 9; 16; 25; 36; 49; 64; 81; 100]
but it returns [1; 4; 9; 4; 9; 16; 9; 16; 25; 16; 25; 36; 25; 36; 49; 36; 49; 64; 49; 64; 81; 64; 81; 100]

Why batch use Seq.windowed?

I expect that implementation (w/out parameters checking):

let mybatch size (sequence : seq<Async<'T>>) : seq<'T> =
    sequence
    |> Seq.split size
    |> Seq.collect (Async.Parallel >> Async.RunSynchronously)

where Seq.split (maybe not best implementation):

module Seq =
    /// Split seq to chunks with size chunkSize (or lesser)
    let split chunkSize (s: #seq<_>) =
        seq {
            let r = ResizeArray<_>()
            for x in s do
                r.Add(x)
                if r.Count = chunkSize then
                    yield r.ToArray() |> Array.toList
                    r.Clear()
            if r.Count <> 0 then yield r.ToArray() |> Array.toList }

By the way, why there is not Seq.split (maybe better name it 'partition'?) in ExtCore. It is very useful function.

jack-pappas commented 9 years ago

Hi @xkrt, you are correct -- using Seq.windowed here gives the wrong result. I'll aim to get it fixed in the next week or so. I'll also add a Seq.segment function like you suggested (ExtCore already has Seq.segmentWith and Seq.segmentBy).