Skip to content

Fix partition on empty arrays #1502

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 10, 2025
Merged

Fix partition on empty arrays #1502

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/impl_methods.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3229,16 +3229,22 @@ impl<A, D: Dimension> ArrayRef<A, D>

let mut result = self.to_owned();

// Must guarantee that the array isn't empty before checking for contiguity
if result.shape().iter().any(|s| *s == 0) {
return result;
}

// Check if the first lane is contiguous
let is_contiguous = result
.lanes_mut(axis)
.into_iter()
.next()
// This unwrap shouldn't cause panics because the array isn't empty
.unwrap()
.is_contiguous();

if is_contiguous {
Zip::from(result.lanes_mut(axis)).for_each(|mut lane| {
result.lanes_mut(axis).into_iter().for_each(|mut lane| {
Comment on lines -3241 to +3247
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() and into_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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I also benchmarked the actual partition line as edited, and the difference is negligible between the two!

lane.as_slice_mut().unwrap().select_nth_unstable(kth);
});
} else {
Expand Down
Loading