Fix partition on empty arrays #1502
Conversation
| Zip::from(result.lanes_mut(axis)).for_each(|mut lane| { | ||
| result.lanes_mut(axis).into_iter().for_each(|mut lane| { |
There was a problem hiding this comment.
@akern40 I know the PR is already merged, but may I ask you why you changed this? Afaik, it does the exact same thing, but it might be slower.
There was a problem hiding this comment.
Good question, and it was maybe a code style choice that I should have just left alone. My feeling is that Zip should be reserved for iterating over multiple arrays, not over a single one. I'm not sure which is faster or slower - we could maybe do a benchmark, it's an interesting question. If I have time today I'll try to set one up.
Thoughts on this coding style? Also, thoughts on my edit? Even if using Zip isn't preferred, should I have just left it alone?
There was a problem hiding this comment.
Hum, I do have trouble finding the source of that belief :) I think it's a mix of:
- I'm pretty sure I have read that Zip is faster in most case, but it's not worth much without a source! I'd ask bluss but I don't want to disturb him.
- I work on 3D and 4D images, so I benchmarked a lot of code in the previous years. I never use
iter()andinto_iter()with ndarray.
I know that our own doc says this:
Prefer higher order methods and arithmetic operations on arrays first, then iteration, and as a last priority using indexed algorithms.
which would make into_iter() a second choice.
As for the "unrelated edit"... I don't really mind. If I believe I'm making the code better, of course I want to do it!
There was a problem hiding this comment.
Ok so two quickly-written benchmarks (sum over 1D array, sum rows over 2D array) show that iter and zip are identical in performance for 2D arrays. Zip has a very slight advantage in summing 1D arrays that is most noticeable when the arrays have 32 or fewer elements.
On reflection, I realized why I even noticed the Zip and thought to change it: I saw Zip on that line, and thought to myself "uh oh, that operation doesn't require lockstep iteration over more than one array! Why is Zip there?" So to me, seeing Zip is an indication of a particular kind of behavior.
There was a problem hiding this comment.
Update: I also benchmarked the actual partition line as edited, and the difference is negligible between the two!
Closes #1501