What am I doing wrong in chunking a list in F#

Question

F# doesn't come easy to me. The following piece of code is supposed to chunk a list. I have no idea what the problem is. Please help.

let chunk items chunkSize =
    let folder = fun state x ->
        match state with (reversedResult, reversedChunk) ->
            if reversedChunk.Length < chunkSize then
                (reversedResult, x::reversedChunk)
            else
                ((reversedChunk |> List.rev)::reversedResult, [x])
    let alsmostDone = items |> List.fold folder ([], [])
    match alsmostDone with
    | (reversedResult, []) -> reversedResult |> List.rev
    | (reversedResult, lastReversedChunk) -> (lastReversedChunk |> List.rev)::reversedResult |> List.rev

enter image description here

score 6 · Accepted Answer · answered Sep 17 '13 at 07:20

6

I think using List.length is a bit more "idiomatic" f#. And then you don't need any type annotations. So:

...
if List.length reversedChunk < chunkSize then
...

answered Sep 17 '13 at 07:20

Lars

456
2
5

score 4 · Answer 2 · answered Sep 17 '13 at 04:57

4

The type of reversedChunk can not be inferred so you need to specify a type. Change your third line to this:

match state with (reversedResult, reversedChunk : list<'T>) ->

answered Sep 17 '13 at 04:57

AnotherDeveloperNamedGreg

577
1
8
12

why does it matter in that context? the intellisense sees it as a list and suggest the .Length property, why doesn't the compiler can do the same? – Trident D'Gao Sep 17 '13 at 05:29
@bonomo - the compiler works strictly from left to right, top to bottom, whilst intellisense works slightly differently – John Palmer Sep 17 '13 at 05:34

score 3 · Answer 3 · answered Sep 17 '13 at 14:54

As others already mentioned, you can use List.length instead of Length to make this work. In practice, it might be better to keep the current chunk length as part of the state, because List.length needs to iterate over the entire list to calculate its length - so you will keep iterating over the chunk each time.

The following is pretty much the same as your original code, but I turned folder into an ordinary function (no need for lambda here) and I removed match (because you can pattern match on the state directly in the function declaration). Then I added reversedChunkSize to the state:

let chunk items chunkSize =
    let folder (reversedResult, reversedChunk, reversedChunkSize) x =
      if reversedChunkSize < chunkSize then
          (reversedResult, x::reversedChunk, reversedChunkSize + 1)
      else
          ((reversedChunk |> List.rev)::reversedResult, [x], 1)
    let alsmostDone = items |> List.fold folder ([], [], 0)
    match alsmostDone with
    | (reversedResult, [], _) -> 
         reversedResult |> List.rev
    | (reversedResult, lastReversedChunk, _) -> 
         (lastReversedChunk |> List.rev)::reversedResult |> List.rev

score 1 · Answer 4 · answered Sep 17 '13 at 06:00

1

This is more to address the first sentence of your question, than the particular coding problem.

Along with (re)inventing from scratch own algorithms FP language mastery is also facilitated by developing an idiomatic thinking of solutions in terms of standard core library functions and combinators. The task you were solving can be achieved by few straightforward idiomatic data transformations:

let chunk size items =                     // test upon chunk 3 [1..6]
    items                                  // [1;2;3;4;5;6]
    |> List.mapi (fun i x -> (i/size,x))   // [(0, 1); (0, 2); (0, 3); (1, 4); (1, 5); (1, 6)]
    |> Seq.groupBy fst                     // seq [(0, seq [(0, 1); (0, 2); (0, 3)]); (1, seq [(1, 4); (1, 5); (1, 6)])]
    |> Seq.map snd                         // seq [seq [(0, 1); (0, 2); (0, 3)]; seq [(1, 4); (1, 5); (1, 6)]]
    |> Seq.map (Seq.map snd >> Seq.toList) // seq [[1; 2; 3]; [4; 5; 6]]
    |> Seq.toList                          // [[1; 2; 3]; [4; 5; 6]]

answered Sep 17 '13 at 06:00

Gene Belitski

10,270
1
34
54

1

Thank you for the advice, however I will stick with my reinvented implementation as it is about 3 times faster. – Trident D'Gao Sep 17 '13 at 06:19
@bonomo: Performance and clarity are not orthogonal properties; although a quality implementation is more often comes off optimization (if such is needed at all) of a clear code, than off refactoring of a fast kludge. – Gene Belitski Sep 17 '13 at 14:07
I hear you, but your code doesn't seem any clearer than mine (at least to me, at least not yet), and it seems like you are doing unnecessary transformations that can be avoided without hurting the clarity – Trident D'Gao Sep 17 '13 at 14:14
by clarity I mean how easy one can understand what's going on, as opposed to being compact yet cryptic – Trident D'Gao Sep 17 '13 at 14:17
@bonomo: I'm with you upon the definition of clarity. From this standpoint the snippet I put together should be crystal clear to readers used to idiomatic F# and closely familiar with core library functions and combinators. On the opposite, your implementation is carrying a bug (list chunks in the final list go from tail to head of `items` list) that came unnoticed for 72 viewers as of now, because it is quite hard to spot this fact just from reading the code. – Gene Belitski Sep 17 '13 at 15:01
I am not sure about the bug, I have a set of tests that says it works right. [] let ``chunk an evenly fit list``() = chunk 3 [1; 2; 3; 4; 5; 6;] |> should equal [[1;2;3]; [4;5;6]] [] let ``chunk a short list``() = chunk 3 [1;2;] |> should equal [[1;2]] [] let ``chunk an uneven list``() = chunk 3 [1;2;3;4;5;] |> should equal [[1;2;3];[4;5]] – Trident D'Gao Sep 17 '13 at 15:05
Using your original code from the question in FSI `chunk [1..5] 3;;` yields `[[4; 5]; [1; 2; 3]]` – Gene Belitski Sep 17 '13 at 15:12
I don't know what you are talking about, I copied the code from the question and ran the test. It works: [] let test = chunkOriginal [1..5] 3 |> should equal [[1;2;3];[4;5]] – Trident D'Gao Sep 17 '13 at 15:18
I see what had happened: `List.rev` in the right end of last line missed my clipboard. **Sorry for the wrong statement about the bug in your code.** – Gene Belitski Sep 17 '13 at 15:40

score 1 · Answer 5 · answered Sep 17 '13 at 09:06

'GetSlice' version:

let chunk2 size items =
    [
        let arrs = items |> Seq.toArray
        let len = List.length items
        for i in [0..size..len-1] do
            let min = System.Math.Min(i+size, len)
            yield arrs.[i..min-1] |> Array.toList
    ]

let a = chunk2 6 [1..10000]

What am I doing wrong in chunking a list in F#

5 Answers5

Linked