Add partition_point #73577

VillSnow · 2020-06-21T09:11:52Z

Add partition_point in C++.
Although existing binary_search in rust does not suitable when the slice has multiple hits,
this function returns exact point of partition.
The definition of this function is very clear and able to accept general matter, therefore you can easily get index which you want like lower/upper_bound.

rust-lang/rfcs#2184

rust-highfive · 2020-06-21T09:11:55Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @LukasKalbertodt (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

src/libcore/slice/mod.rs

arthurprs · 2020-06-23T09:40:18Z

Does it make sense for this to return 2 slices instead?

VillSnow · 2020-06-23T12:38:01Z

Does it make sense for this to return 2 slices instead?

I'm not sure. It is easy to convert index to two slices, or two slices to index. There is almost no difference.

The two slices is consistent with Iterator::partition but it is because iterator should not be just consumed.

binary_search returns index and also Iterator::partition_in_place substantially returns index.

I think the index simple and is better.

Amanieu · 2020-06-23T17:06:46Z

LGTM, can you create a tracking issue and update the stability attribute to point to it?

VillSnow · 2020-06-24T14:11:08Z

I'll be inactive a few days so I make tracking issue after that.

LukasKalbertodt

Seems like a potentially useful method. I'm fine with adding it unstably. Just a few inline comments.

LukasKalbertodt · 2020-06-24T21:35:07Z

src/libcore/slice/mod.rs

+
+        while left != right {
+            let mid = left + (right - left) / 2;
+            let value = unsafe { self.get_unchecked(mid) };


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:).

src/libcore/slice/mod.rs

LukasKalbertodt · 2020-06-24T21:56:48Z

src/libcore/slice/mod.rs

+    /// Returns index of partition point according to the given predicate,
+    /// such that all those that return true precede the index and
+    /// such that all those that return false succeed the index.
+    ///
+    /// The slice must be partitioned
+    /// so that all elements where the predicate returns true
+    /// precede the elements where the predicate returns false.


I think this text could be improved. In particular, I had to check the example to understand whether the returned index is the last index of the first half or the first index of the second half. One suggestion:

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, as this method performs a kind of binary search.

EFanZh · 2020-06-27T12:54:18Z

There is a faster binary search algorithm implemented in #45333 used by binary_search_by. Also, if this gets merged, there will be two pieces of binary search codes. Maybe implement the binary search algorithm in another function and have both binary_search_by and partition_point calling it?

Co-authored-by: Lukas Kalbertodt <[email protected]>

VillSnow · 2020-06-28T07:50:47Z

Though I'm not sure which is better Amanieu's idea or LukasKalbertodt 's idea, I adopted LukasKalbertodt's idea for consistence

VillSnow · 2020-06-28T08:10:35Z

@EFanZh I don't think the old binary_search code is same as my code and rather similar to new code.
I haven't tested which is faster my code or binary_search code.

Yes binary_search and partition_point can use common base function and should do in the furure. My first objective was to include pertiton_point in std.

src/libcore/slice/mod.rs

Co-authored-by: Amanieu d'Antras <[email protected]>

Amanieu · 2020-06-28T12:32:15Z

@bors r+

bors · 2020-06-28T12:32:17Z

📌 Commit 6f8ad3b has been approved by Amanieu

@ghost

…arth Rollup of 9 pull requests Successful merges: - rust-lang#73577 (Add partition_point) - rust-lang#73757 (Const prop: erase all block-only locals at the end of every block) - rust-lang#73774 (Make liveness more precise for assignments to fields) - rust-lang#73795 (Add some `const_compare_raw_pointers`-related regression tests) - rust-lang#73800 (Forward Hash::write_iN to Hash::write_uN) - rust-lang#73813 (Rename two `Resolver` traits) - rust-lang#73817 (Rename clashing_extern_decl to clashing_extern_declarations.) - rust-lang#73826 (Fix docstring typo) - rust-lang#73833 (Remove GlobalCtxt::enter_local) Failed merges: r? @ghost

Folyd · 2020-07-03T14:49:20Z

src/libcore/slice/mod.rs

+            // SAFETY:
+            // When left < right, left <= mid < right.
+            // Therefore left always increases and right always decreases,
+            // and eigher of them is selected.


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

oops thank you.

Folyd · 2020-07-03T15:01:39Z

src/libcore/slice/mod.rs

+    /// 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,


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

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.

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.

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.

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

Folyd · 2020-07-03T15:07:21Z

Hi @VillSnow. Thanks for this great PR. However, I found two things that deserved to improve. Please check it out. Thanks! 😺

… r=matklad Stabilize the partition_point feature Stabilize the partition_point feature. Tracking Issue: rust-lang#73831 First PR: rust-lang#73577

Add partition_point

4c8ce48

rust-highfive assigned LukasKalbertodt Jun 21, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 21, 2020

fix: doc test

c9b4915

matklad added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 21, 2020

nagisa reviewed Jun 21, 2020

View reviewed changes

src/libcore/slice/mod.rs Outdated Show resolved Hide resolved

VillSnow added 2 commits June 22, 2020 21:35

update: doc comment

27b06f1

add: tests

8cc6998

LukasKalbertodt suggested changes Jun 24, 2020

View reviewed changes

LukasKalbertodt added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 24, 2020

VillSnow and others added 4 commits June 28, 2020 16:25

Add comment on use of unsafe

52f9762

Update src/libcore/slice/mod.rs

9335787

Co-authored-by: Lukas Kalbertodt <[email protected]>

Merge branch 'master' of https://github.com/VillSnow/rust

83d5998

Update doc comment

d720a19

VillSnow mentioned this pull request Jun 28, 2020

Tracking Issue for partition_point #73831

Closed

5 tasks

Update tracking issue number

60f2ba2

Amanieu reviewed Jun 28, 2020

View reviewed changes

src/libcore/slice/mod.rs Outdated Show resolved Hide resolved

Update src/libcore/slice/mod.rs

6f8ad3b

Co-authored-by: Amanieu d'Antras <[email protected]>

bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 28, 2020

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 28, 2020

Manishearth mentioned this pull request Jun 28, 2020

Rollup of 9 pull requests #73838

Merged

bors merged commit ec48989 into rust-lang:master Jun 28, 2020

Folyd reviewed Jul 3, 2020

View reviewed changes

VillSnow mentioned this pull request Jul 4, 2020

fix typo #74032

Closed

VillSnow mentioned this pull request Jan 14, 2021

Stabilize the partition_point feature #81012

Merged

cuviper added this to the 1.46 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add partition_point #73577

Add partition_point #73577

VillSnow commented Jun 21, 2020

rust-highfive commented Jun 21, 2020

arthurprs commented Jun 23, 2020

VillSnow commented Jun 23, 2020

Amanieu commented Jun 23, 2020

VillSnow commented Jun 24, 2020

LukasKalbertodt left a comment

LukasKalbertodt Jun 24, 2020

LukasKalbertodt Jun 24, 2020

EFanZh commented Jun 27, 2020

VillSnow commented Jun 28, 2020

VillSnow commented Jun 28, 2020

Amanieu commented Jun 28, 2020

bors commented Jun 28, 2020

Folyd Jul 3, 2020

VillSnow Jul 3, 2020

Folyd Jul 3, 2020 •

edited

Loading

matklad Jul 3, 2020

VillSnow Jul 3, 2020

Folyd Jul 3, 2020

VillSnow Jul 3, 2020

Folyd commented Jul 3, 2020

Add partition_point #73577

Add partition_point #73577

Conversation

VillSnow commented Jun 21, 2020

rust-highfive commented Jun 21, 2020

arthurprs commented Jun 23, 2020

VillSnow commented Jun 23, 2020

Amanieu commented Jun 23, 2020

VillSnow commented Jun 24, 2020

LukasKalbertodt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EFanZh commented Jun 27, 2020

VillSnow commented Jun 28, 2020

VillSnow commented Jun 28, 2020

Amanieu commented Jun 28, 2020

bors commented Jun 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Folyd Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Folyd commented Jul 3, 2020

Folyd Jul 3, 2020 •

edited

Loading