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

[API Proposal]: Add a Remove method to PriorityQueue #93925

Closed
eiriktsarpalis opened this issue Oct 24, 2023 · 3 comments · Fixed by #93994
Closed

[API Proposal]: Add a Remove method to PriorityQueue #93925

eiriktsarpalis opened this issue Oct 24, 2023 · 3 comments · Fixed by #93994
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections
Milestone

Comments

@eiriktsarpalis
Copy link
Member

Background and motivation

When we shipped the PriorityQueue class back in .NET 6 we made the explicit decision to not add support for efficient, $\mathcal O(\log n)$ priority updates. This was due to priority updates not being supported without sacrificing overall performance and lack of widespread use in .NET codebases. We have a separate issue that tracks adding a separate heap type that does support priority updates.

Even though the PriorityQueue type itself cannot be retrofitted to support efficient priority updates, it can be made to support $\mathcal O(n)$ priority updates. This can be useful in educational contexts or coding interviews, where the actual asymptotic performance is not mission critical and can be overlooked. Put differently, users have asked for this facility so that they can implement Dijkstra's algorithm in leetCode using C#.

In the case of the Java, users can encode priority updates in the PriorityQueue class by piggybacking on its built-in remove method.

API Proposal

namespace System.Collections.Generic;

public partial class PriorityQueue<TElement, TPriority>
{
    // Scans the heap for elements equal to the provided value and removes the first occurrence.
    public bool Remove(TElement element, out TPriority priority, IEqualityComparer<TElement>? equalityComparer = null);
}

API Usage

It's possible to use the above API to encode a $\mathcal O(n)$ priority update as follows:

public static void EnqueueOrUpdate<TElement, TPriority>(this PriorityQueue<TElement, TPriority> queue, TElement element, TPriority priority)
{
    queue.Remove(element, out _);
    queue.Enqueue(element, priority);
}

Alternative Designs

No response

Risks

It could end up being used in production code. I expect this risk would be mitigated since we're not explicitly adding an "update" API, instead users need to invoke a Remove API that will be explicitly documented as a linear-time scan operation.

@eiriktsarpalis eiriktsarpalis added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 24, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 24, 2023
@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections untriaged New issue has not been triaged by the area owner labels Oct 24, 2023
@eiriktsarpalis eiriktsarpalis self-assigned this Oct 24, 2023
@ghost
Copy link

ghost commented Oct 24, 2023

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

Issue Details

Background and motivation

When we shipped the PriorityQueue class back in .NET 6 we made the explicit decision to not add support for efficient, $\mathcal O(\log n)$ priority updates. This was due to priority updates not being supported without sacrificing overall performance and lack of widespread use in .NET codebases. We have a separate issue that tracks adding a separate heap type that does support priority updates.

Even though the PriorityQueue type itself cannot be retrofitted to support efficient priority updates, it can be made to support $\mathcal O(n)$ priority updates. This can be useful in educational contexts or coding interviews, where the actual asymptotic performance is not mission critical and can be overlooked. Put differently, users have asked for this facility so that they can implement Dijkstra's algorithm in leetCode using C#.

In the case of the Java, users can encode priority updates in the PriorityQueue class by piggybacking on its built-in remove method.

API Proposal

namespace System.Collections.Generic;

public partial class PriorityQueue<TElement, TPriority>
{
    // Scans the heap for elements equal to the provided value and removes the first occurrence.
    public bool Remove(TElement element, out TPriority priority, IEqualityComparer<TElement>? equalityComparer = null);
}

API Usage

It's possible to use the above API to encode a $\mathcal O(n)$ priority update as follows:

public static void EnqueueOrUpdate<TElement, TPriority>(this PriorityQueue<TElement, TPriority> queue, TElement element, TPriority priority)
{
    queue.Remove(element, out _);
    queue.Enqueue(element, priority);
}

Alternative Designs

No response

Risks

It could end up being used in production code. I expect this risk would be mitigated since we're not explicitly adding an "update" API, instead users need to invoke a Remove API that will be explicitly documented as a linear-time scan operation.

Author: eiriktsarpalis
Assignees: -
Labels:

api-suggestion, area-System.Collections, untriaged

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 9.0.0 milestone Oct 24, 2023
@bartonjs
Copy link
Member

bartonjs commented Oct 24, 2023

Video

  • We added an out for the removed element to forestall future requests for it.
  • We added some nullability annotations.
namespace System.Collections.Generic;

public partial class PriorityQueue<TElement, TPriority>
{
    // Scans the heap for elements equal to the provided value and removes the first occurrence.
    public bool Remove(
        TElement element,
        [MaybeNullWhen(false)] out TElement removedElement,
        [MaybeNullWhen(false)] out TPriority priority,
        IEqualityComparer<TElement>? equalityComparer = null);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented area-System.Collections and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Oct 24, 2023
@ghost
Copy link

ghost commented Oct 24, 2023

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

Issue Details

Background and motivation

When we shipped the PriorityQueue class back in .NET 6 we made the explicit decision to not add support for efficient, $\mathcal O(\log n)$ priority updates. This was due to priority updates not being supported without sacrificing overall performance and lack of widespread use in .NET codebases. We have a separate issue that tracks adding a separate heap type that does support priority updates.

Even though the PriorityQueue type itself cannot be retrofitted to support efficient priority updates, it can be made to support $\mathcal O(n)$ priority updates. This can be useful in educational contexts or coding interviews, where the actual asymptotic performance is not mission critical and can be overlooked. Put differently, users have asked for this facility so that they can implement Dijkstra's algorithm in leetCode using C#.

In the case of the Java, users can encode priority updates in the PriorityQueue class by piggybacking on its built-in remove method.

API Proposal

namespace System.Collections.Generic;

public partial class PriorityQueue<TElement, TPriority>
{
    // Scans the heap for elements equal to the provided value and removes the first occurrence.
    public bool Remove(TElement element, out TPriority priority, IEqualityComparer<TElement>? equalityComparer = null);
}

API Usage

It's possible to use the above API to encode a $\mathcal O(n)$ priority update as follows:

public static void EnqueueOrUpdate<TElement, TPriority>(this PriorityQueue<TElement, TPriority> queue, TElement element, TPriority priority)
{
    queue.Remove(element, out _);
    queue.Enqueue(element, priority);
}

Alternative Designs

No response

Risks

It could end up being used in production code. I expect this risk would be mitigated since we're not explicitly adding an "update" API, instead users need to invoke a Remove API that will be explicitly documented as a linear-time scan operation.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

api-approved, area-System.Collections

Milestone: 9.0.0

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 25, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 30, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants