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

Why doesn't PriorityQueue have the Remove(TElement element) method #65928

Closed
sw47 opened this issue Feb 27, 2022 · 7 comments
Closed

Why doesn't PriorityQueue have the Remove(TElement element) method #65928

sw47 opened this issue Feb 27, 2022 · 7 comments
Labels
area-System.Collections untriaged New issue has not been triaged by the area owner

Comments

@sw47
Copy link

sw47 commented Feb 27, 2022

The current PriorityQueue Implementation for .NET PriorityQueue Implementation for .NET does not have the Remove(TElement element) method as written on the original contract defined in this #14032

I perused the history of #14032 spanning half a decade+ worth of discussions (!) and unfortunately didn't find the post which proposed to remove the the Remove method from the final implementation.

Can someone shed some light on this? I would have expected the .NET Priority Queue implementation to have a certain degree of parity to [Java's implementation] (https://docs.oracle.com/javase/7/docs/api/java/util/PriorityQueue.html)

Also, +💯 this #14032 (comment) - would be great if the history of this API was written up as a blog entry on DevDiv :)

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Collections untriaged New issue has not been triaged by the area owner labels Feb 27, 2022
@ghost
Copy link

ghost commented Feb 27, 2022

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

The current PriorityQueue Implementation for .NET PriorityQueue Implementation for .NET does not have the Remove(TElement element) method as written on the original contract defined in this #14032

I perused the history of #14032 spanning half a decade+ worth of discussions (!) and unfortunately didn't find the post which proposed to remove the the Remove method from the final implementation.

Can someone shed some light on this? I would have expected the .NET Priority Queue implementation to have a certain degree of parity to [Java's implementation] (https://docs.oracle.com/javase/7/docs/api/java/util/PriorityQueue.html)

Also, +💯 this #14032 (comment) - would be great if the history of this API was written up as a blog entry on DevDiv :)

Author: sw47
Assignees: -
Labels:

area-System.Collections, untriaged

Milestone: -

@Joe4evr
Copy link
Contributor

Joe4evr commented Feb 27, 2022

The Priority Queue was implemented according to the API proposed at #43957, which states:

  • Should we include a void Remove(TElement element); method?
    • No. Can be ambiguous operation in the presence of duplicate elements.

@sw47
Copy link
Author

sw47 commented Feb 27, 2022

Thanks @Joe4evr - what was the design rationale for not allowing duplicates? PriorityQueues in Java allow duplicates....

@jnyrup
Copy link
Contributor

jnyrup commented Feb 27, 2022

PriorityQueue supports enqueuing duplicates, the ambiguity comes when removing.

var pq = new PriorityQueue<int, int>();
pq.Enqueue(1, 1);
pq.Enqueue(1, 2);
// pq.Remove(1); <-- which of the above should be removed?

@Genbox
Copy link

Genbox commented Feb 27, 2022

which of the above should be removed?

Presumably the one where the equality comparer determines equality. Sure, the default comparer for integer means that 1 equals 1, but in collections, 1 is not an integer - it is a key - and keys do not necessarily have value equality.

Since .NET has a very simplistic equality comparer interface where it is only possible to compare keys and not position/key hash/priority/value, it gives the developer some difficult decisions with potential side effects when it comes to duplicates. Therefore I agree that it is not exactly straightforward, but also not an issue that the data structure necessarily should concern itself with.

However, it makes me think that a RemoveAt() method would be suitable and is a paradigm in .NET collections already. Was RemoveAt() discussed before?

@theodorzoulias
Copy link
Contributor

This issue might be a duplicate of this: Add a priority queue that supports priority updates

@eiriktsarpalis
Copy link
Member

It might be worth pointing out that the Remove method in Java's PriorityQueue is an O(n) operation, so using it to express priority updates (as is sometimes recommended in public forums) can be prohibitively expensive. If we do offer such a solution we might consider offering O(log n) operations, but that requires a different implementation: #44871

Closing in favor of #44871.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Collections untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

6 participants