Fix duplicated iteration issue of Enumerable#in_groups_of #3128
Conversation
| ary = Array(T | U).new(size, filled_up_with) | ||
| end | ||
| each_slice(size) do |slice| | ||
| group = slice.size < size ? slice + [filled_up_with] : slice |
There was a problem hiding this comment.
Mhh, this needs to fill it up with size - slice.size elements, no?
There was a problem hiding this comment.
Ah, does it mean assert { [1, 2, 3, 4].in_groups_of(3).should eq([[1, 2, 3], [4, nil, nil]]) }?
It sounds so reasonable to me, but current behavior does not work for it. Would I fix the behavior in this PR?
There was a problem hiding this comment.
Ah, current code works for it ... 🙇
There was a problem hiding this comment.
Yes, Rails/ActiveSupport's version does so anyway:
irb(main):001:0> [1, 2, 3, 4].in_groups_of(3)
=> [[1, 2, 3], [4, nil, nil]]
| yield group | ||
| else | ||
| yield slice | ||
| end |
There was a problem hiding this comment.
How about just
(size - slice.size).times { slice << filled_up_with }
yield sliceOr perhaps
slice.concat Array(T|U).new(size - slice.size, filled_up_with)
yield slice? It would be interesting to see what's more expensive (slower), potentially repeated resizings of pushing n-m times or allocating a whole new array. Actually does Array#fill resize? I can't remember.
There was a problem hiding this comment.
Thank you! The code looks simple to me.
But I think the slice is declared to Array(T) at
Line 160 in 0cc341d
Can I append the nil or given value to the slice at here?
I have tried some code as below they show errors.
group = slice.as(Array(T | U)) # => can't cast Array(Int32) to Array(Int32 | Nil)There was a problem hiding this comment.
Oh, you're right, I forgot about that. How about
unless slice.size == size
group = Array(T|U).new(size, filled_up_with)
slice.each_with_index {|item, index| group[index] = item }
slice = group
end
yield sliceThere was a problem hiding this comment.
Sounds perfect to me!
I copied the logic in 1c1334f.
Thank you!
(Style adjusted to standardize naming for the returned variable 🙇 )
|
Can you provide an example when it fails with the current crystal version? |
|
@benoist It's not about failure, it's about the Enumerable being traversed twice. It can probably be tested with a custom enumerable. |
|
Ah that makes more sense :) |
|
Thank you! But the implementation is not correct, and I think once we improve generics + covariance and contravariance this will give an error. The problem is that I think the proper way to implement this is to make a third private method that both |
|
Also, benchmarking via |
|
😨 Thank you for sharing with me! I have updated some code, is it correct for your pointed out? Then I could choose #3128 (comment) way. And benchmarked as below. Is it enough for the fairness? echo '1000.times { (0...400000).in_groups_of(3, "padding") }' > bench.cr
git checkout 31562ca54be262f60fee941ec859f86a699ea461
time bin/crystal run --release bench.cr
#=> bin/crystal run --release bench.cr 8.71s user 0.99s system 129% cpu 7.480 total
git checkout 29667d4ad394ec66fa0b88bdb9bc427a81675ed0
time bin/crystal run --release bench.cr
#=> bin/crystal run --release bench.cr 6.73s user 0.81s system 111% cpu 6.774 total |
|
@kachick Looks good, thank you! |
|
Thank you! 😂 |
No description provided.