Skip to content
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.

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

Add partition_point #73577

Merged
merged 10 commits into from
Jun 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
54 changes: 54 additions & 0 deletions src/libcore/slice/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2663,6 +2663,60 @@ impl<T> [T] {
{
self.iter().is_sorted_by_key(f)
}

/// Returns the index of the partition point according to the given predicate
/// (the index of the first element of the second partition).
///
/// The slice is assumed to be partitioned according to the given predicate.
/// This means that all elements for which the predicate returns true are at the start of the slice
/// and all elements for which the predicate returns false are at the end.
/// For example, [7, 15, 3, 5, 4, 12, 6] is a partitioned under the predicate x % 2 != 0
/// (all odd numbers are at the start, all even at the end).
///
/// If this slice is not partitioned, the returned result is unspecified and meaningless,
Copy link
Contributor

@Folyd Folyd Jul 3, 2020

Choose a reason for hiding this comment

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

/// If this slice is not partitioned, the returned result is unspecified and meaningless,
/// as this method performs a kind of binary search.

I think we'd better change to "If this slice is not sorted ..." as we actually want to tell the end-user an ordered slice is required for this method.

Copy link
Member

Choose a reason for hiding this comment

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

The comment is right -- the slice has to be partitioned, and not ordered. Raison d'être of partition_point is that it works with non-Ord things. To make this more precise, we might link to https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html#method.is_partitioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed in 95% cases partition_point is used to get lower/upper_bound. However in 5% cases it looks more partition point than lower bound.
For example, we can get all valid values from [3, 1, 4, 1, 5, -1, -1, -1, -1, -1].
Actually this array is sorted by |&x| x != -1 in descending order (true is larger than false), but many user would think that the array is partitioned at index 5 by whether -1 or not.

What do you think about adding like this?

Sorted slice in ascending order is always partitioned by whether less than any value, and in descending order is always partitioned by whether greater than any value. For example, [1, 2, 4, 8, 16] is partitioned by |&x| x < -10, |&x| x < 2, |&x| x < 5, and |&x| x < -100 and other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the kind explanation and sorry for my misunderstanding. Yes, the slice requires partitioned instead of sorted.

However, I still confused by the comment If this slice is not partitioned, the returned result is unspecified and meaningless. If we did call this method on a not partitioned slice, the result should be an Option<size> rather than an unspecified or meaningless value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Binary search runs in O(log n) by assuming ordered. If it checks whether is sorted, it must slower than O(n).

/// as this method performs a kind of binary search.
///
/// # Examples
///
/// ```
/// #![feature(partition_point)]
///
/// let v = [1, 2, 3, 3, 5, 6, 7];
/// let i = v.partition_point(|&x| x < 5);
///
/// assert_eq!(i, 4);
/// assert!(v[..i].iter().all(|&x| x < 5));
/// assert!(v[i..].iter().all(|&x| !(x < 5)));
/// ```
#[unstable(feature = "partition_point", reason = "new API", issue = "73831")]
pub fn partition_point<P>(&self, mut pred: P) -> usize
where
P: FnMut(&T) -> bool,
{
let mut left = 0;
let mut right = self.len();

while left != right {
let mid = left + (right - left) / 2;
// SAFETY:
// When left < right, left <= mid < right.
// Therefore left always increases and right always decreases,
// and eigher of them is selected.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think eigher is a typo? It should be either :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops thank you.

// In both cases left <= right is satisfied.
// Therefore if left < right in a step,
// left <= right is satisfied in the next step.
// Therefore as long as left != right, 0 <= left < right <= len is satisfied
// and if this case 0 <= mid < len is satisfied too.
let value = unsafe { self.get_unchecked(mid) };
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment about why this unsafe is fine. You can probably take the text from binary_search. Also see #66219 (let the comment start with // SAFETY:).

if pred(value) {
left = mid + 1;
} else {
right = mid;
}
}

left
}
}

#[lang = "slice_u8"]
Expand Down
1 change: 1 addition & 0 deletions src/libcore/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#![feature(const_forget)]
#![feature(option_unwrap_none)]
#![feature(peekable_next_if)]
#![feature(partition_point)]

extern crate test;

Expand Down
40 changes: 40 additions & 0 deletions src/libcore/tests/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,46 @@ fn test_binary_search_implementation_details() {
assert_eq!(b.binary_search(&3), Ok(8));
}

#[test]
fn test_partition_point() {
let b: [i32; 0] = [];
assert_eq!(b.partition_point(|&x| x < 5), 0);

let b = [4];
assert_eq!(b.partition_point(|&x| x < 3), 0);
assert_eq!(b.partition_point(|&x| x < 4), 0);
assert_eq!(b.partition_point(|&x| x < 5), 1);

let b = [1, 2, 4, 6, 8, 9];
assert_eq!(b.partition_point(|&x| x < 5), 3);
assert_eq!(b.partition_point(|&x| x < 6), 3);
assert_eq!(b.partition_point(|&x| x < 7), 4);
assert_eq!(b.partition_point(|&x| x < 8), 4);

let b = [1, 2, 4, 5, 6, 8];
assert_eq!(b.partition_point(|&x| x < 9), 6);

let b = [1, 2, 4, 6, 7, 8, 9];
assert_eq!(b.partition_point(|&x| x < 6), 3);
assert_eq!(b.partition_point(|&x| x < 5), 3);
assert_eq!(b.partition_point(|&x| x < 8), 5);

let b = [1, 2, 4, 5, 6, 8, 9];
assert_eq!(b.partition_point(|&x| x < 7), 5);
assert_eq!(b.partition_point(|&x| x < 0), 0);

let b = [1, 3, 3, 3, 7];
assert_eq!(b.partition_point(|&x| x < 0), 0);
assert_eq!(b.partition_point(|&x| x < 1), 0);
assert_eq!(b.partition_point(|&x| x < 2), 1);
assert_eq!(b.partition_point(|&x| x < 3), 1);
assert_eq!(b.partition_point(|&x| x < 4), 4);
assert_eq!(b.partition_point(|&x| x < 5), 4);
assert_eq!(b.partition_point(|&x| x < 6), 4);
assert_eq!(b.partition_point(|&x| x < 7), 4);
assert_eq!(b.partition_point(|&x| x < 8), 5);
}

#[test]
fn test_iterator_nth() {
let v: &[_] = &[0, 1, 2, 3, 4];
Expand Down