Fork me on GitHub
#clojure-dev
<
2020-12-09
>
Vincent Cantin09:12:27

Is it a bug?

(partition 3 1 [:a :b :c] (range 5))
; => ((0 1 2) (1 2 3) (2 3 4) (3 4 :a))
I would expect ((0 1 2) (1 2 3) (2 3 4)) as a result instead.

delaguardo09:12:34

Isn’t it working exactly as described in docstring?

Vincent Cantin10:12:01

The docstring is ambiguous on this point.

Vincent Cantin10:12:30

as a comparison with partition-all, there is an inconsistency.

(partition-all 3 1 (range 5))
=> ((0 1 2) (1 2 3) (2 3 4) (3 4) (4))

Vincent Cantin10:12:32

if partition was behaving consistently with partition-all, it should be:

(partition 3 1 [:a :b :c] (range 5))
; => ((0 1 2) (1 2 3) (2 3 4) (3 4 :a) (4 :a :b))

delaguardo10:12:54

hm… but docstring for partition is not mentioning partition-all however I can see some problems with that sentence:

If a pad collection is supplied, use its elements as
necessary to complete last partition upto n items.
there is no definition what is last partition

Vincent Cantin10:12:39

there should be an emphasis on the "last partition" with no s

Vincent Cantin10:12:54

But yeah ... the last partition is not defined.

delaguardo10:12:22

of it could be first incomplete partition

Vincent Cantin11:12:57

(partition 3 3 [:a :b :c] (range 9))
=> ((0 1 2) (3 4 5) (6 7 8))
Your formulation does not apply.

Vincent Cantin11:12:35

I think that it is a bug.

delaguardo11:12:58

what do you expect?

Vincent Cantin11:12:07

I would expect the "last partition" to be: > > The last partition to contain elements from the input collection (not the padding collection) which are not present in any previous partition.

delaguardo11:12:50

but this (6 7 8) is that partition, isn’t it?

Vincent Cantin11:12:52

It would mean:

(partition 3 3 [:a :b :c] (range 10))
; => ((0 1 2) (3 4 5) (6 7 8) (9 :a :b)) ; actual behavior

(partition 3 3 [:a :b :c] (range 9))
; => ((0 1 2) (3 4 5) (6 7 8)) ; actual behavior

(partition 3 1 [:a :b :c] (range 5)) ; suspected bug here
; => ((0 1 2) (1 2 3) (2 3 4) (3 4 :a)) ; actual behavior
; => ((0 1 2) (1 2 3) (2 3 4)) ; not actual behavior, but that's what I expect.

Vincent Cantin11:12:02

Does it make sense to you?

delaguardo11:12:40

(partition 3 1 [:a :b :c] (range 5))
;; ((0 1 2) (1 2 3) (2 3 4) (3 4) (3))
;;  ^_____________________^ ^_______^
;;  complete partitions     those are incomplete
;;  each of size 3          size != 3
;;  each at offset apart    each at offset apart
two last partitions partition should drop because it against first part of the docstring but we supply third argument (pad) so we should read carefully second part. If a pad collection is supplied, use its elements as necessary to complete last partition upto n items. so we must complete last partition using elements in pad to do that we need to define last partition and because there is no definition in the docstring we have to guess If you consider my suggestion: first incomplete partition will be (3 4) then this “incomplete partition” should be completed using element from pad list. as a result: (3 4 :a) so this is correct behavior for me as I understand docstring for partition function

delaguardo11:12:29

but also my english skills are not sharp enough to suggest the wording for clojure.core )

delaguardo11:12:58

I just tried to give my personal understanding of the function

Vincent Cantin11:12:49

I see ... but in practice, it is useless to return (3 4 :a) because 3 and 4 were already returned in the previous partition. So IMHO, it is still fishy/buggy.

delaguardo12:12:40

this is still useful in case nil-padding is not desired (which makes usage of partition-all useless) something like (partition 3 1 (repeat 2 :n-a) '(1 nil 3 nil 4 nil 5 nil 6 nil)) here I want to clear the difference between nil value coming from my list and an indicator of absence of value because “not enough elements to fill the partition”

favila12:12:45

I would want and expect current behavior. It seems fishy because the partitions are overlapping and you are thinking of items not “windows”. I rely on this behavior to give me a sliding window over a seq that fills in the tail.

Vincent Cantin12:12:32

@favila are you using different values for n and step ? I want to make sure that we are talking about the same thing.

favila12:12:30

So in your example, every element that would be collected by “step” defines a partition

favila12:12:12

If there are less than n items left after it, it is normally dropped, unless enough pad is supplied

favila12:12:58

> I see ... but in practice, it is useless to return (3 4 :a) because 3 and 4 were already returned in the previous partition. So IMHO, it is still fishy/buggy.

favila12:12:01

I’m saying no, because 3 was not in the first position of the previous partition

Vincent Cantin12:12:49

If the function was following this logic, there should be another partition (4 :a :b) after that

favila12:12:50

I actually expect an additional (4 :b :c), I’m not sure why there isn’t one

favila12:12:23

I guess the logic is you will only get one incomplete partition

favila12:12:49

And yeah, that breaks how I use partition for windowing if n>2

Vincent Cantin12:12:00

So we agree that there is a problem, there?

favila12:12:30

Yeah but we have opposite conceptions of what is the correct behavior :)

favila12:12:04

i don’t think current behavior violates the doc though. I just would like it to go further

favila12:12:42

But definitely the “step” not “n” defines the partitions, not whether an item was consumed