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 doc aliases for BinaryHeap pushpop and heapify implementations #84939

Closed
wants to merge 2 commits into from
Closed

Add doc aliases for BinaryHeap pushpop and heapify implementations #84939

wants to merge 2 commits into from

Conversation

lopopolo
Copy link
Contributor

@lopopolo lopopolo commented May 5, 2021

BinaryHeap implements some common heap operations, like heapify and pushpop with non-standard names like From<Vec<T>> and peek_mut + DerefMut.

This PR follows up on #28147 and adds doc aliases to From<Vec<T>> as heapify and peek_mut as pushpop [0].

[0]: Based on its name in Python - https://docs.python.org/3/library/heapq.html#heapq.heappushpop

Docs Screenshots

image

image

BinaryHeap implements some common heap operations, like `heapify` and
`pushpop` with non-standard names like `From<Vec<T>>` and `peek_mut` +
`DerefMut`.

This PR follows up on #28147 and adds doc aliases to
`From<Vec<T>>` as _heapify_ and `peek_mut` as _pushpop_ [0].

[0]: Based on its name in Python - https://docs.python.org/3/library/heapq.html#heapq.heappushpop
@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 5, 2021
@lopopolo
Copy link
Contributor Author

lopopolo commented May 5, 2021

While I'm adding doc aliases, would making binary heap discoverable by the priority queue name from C++ be a welcome change?

Edit: Be the change you want to see in the world

image

compared to the current stable docs:

image

@nagisa
Copy link
Member

nagisa commented May 7, 2021

It isn't obvious to me if peek_mut is in fact the same as pushpop. pushpop can pop the same element that has been just pushed if its the smallest element, whereas the *(bh.peek_mut()) = val would first "pop" the heap's top element and push a new one, behaving more like poppush.

(As a side note, how prevalent is the terminology used by Python? heapq seems like it'd be a pretty obscure module, comparatively)

@lopopolo
Copy link
Contributor Author

lopopolo commented May 7, 2021

pushpop is a swap operation on the head node in the heap + a sift down. It requires a read before the write.

PeekMut can be used to implement pushpop like this:

if let Some(mut peek) = heap.peek_mut() {
    let popped = std::mem::replace(&mut peek, pushed);
    Some(popped)
} else {
    None
}

I do agree that a more ergonomic pushpop would be fn pushpop(self, pushed: T) -> T on PeekMut.

(As a side note, how prevalent is the terminology used by Python? heapq seems like it'd be a pretty obscure module, comparatively)

This was also the terminology used in the tracking issue linked in this PR description.

@nagisa
Copy link
Member

nagisa commented May 7, 2021

The difference is specifically in the order of the pop and push operations that happen between Python and peek_mut based code, making these operations fundamentally different.

@lopopolo
Copy link
Contributor Author

lopopolo commented May 7, 2021

pushpop in the python heapq module does not do a heap.push() followed by a heap.pop(). It is an API that optimizes those two sequential operations into one by comparing the given argument with the head of the heap and is equivalent to calling mem::swap(pushed, heap.head) followed by a sift down if necessary.

I've updated the above code to reflect that pushpop can be implemented with std::mem::replace on PeekMut.

This is the implementation of heappushpop in CPython -- https://github.com/python/cpython/blob/3.9/Lib/heapq.py#L161-L166:

def heappushpop(heap, item):
    """Fast version of a heappush followed by a heappop."""
    if heap and heap[0] < item:
        item, heap[0] = heap[0], item
        _siftup(heap, 0)
    return item

@lopopolo
Copy link
Contributor Author

lopopolo commented May 7, 2021

My motivation for adding the pushpop doc alias to peek_mut is because I was starting with the Python heap code and the only reference to this behavior in BinaryHeap is the above tracking issue which decided to deprecate BinaryHeap::push_pop because it can be implemented with BinaryHeap::peek_mut, but it took a bit of work to find that old tracking issue and make the connection.

Even if the documentation of BinaryHeap::peek_mut mentions this, it would still not be discoverable in std's documentation search.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2021
@lopopolo
Copy link
Contributor Author

lopopolo commented Jun 8, 2021

@kennytm is there a desire to move forward with this PR?

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2021
@Dylan-DPC-zz
Copy link

r? @Mark-Simulacrum

@kennytm
Copy link
Member

kennytm commented Jun 27, 2021

cc rust-lang/libs-team#18

@Mark-Simulacrum
Copy link
Member

I'm going to mark this as S-blocked on rust-lang/libs-team#18, cc @m-ou-se

@Mark-Simulacrum Mark-Simulacrum added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 28, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jul 28, 2021

We have a policy now for doc aliases, so this is no longer blocked: https://std-dev-guide.rust-lang.org/documentation/doc-alias-policy.html

However, these aliases do not fit within that policy, so I'm closing this PR.

@m-ou-se m-ou-se closed this Jul 28, 2021
@lopopolo lopopolo deleted the binaryheap-pushpop-doc-alias branch October 9, 2022 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants