Skip to content

Use Base.Ordering with heaps #646

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

Closed
wants to merge 4 commits into from

Conversation

jlumpe
Copy link
Contributor

@jlumpe jlumpe commented Aug 3, 2020

Note: this is basically a duplicate of #547, which still seems active. I wrote all this before finding that one, so I'm submitting it anyways just in case any of it is still useful.

Changes made:

  • comp::Comp arguments and fields replaced with order::Base.Ordering everywhere
    • Heaps now support arbitrary orderings
    • LessThan and GreaterThan singleton types and compare function replaced with Base.Forward, Base.Reverse, and Base.Order.lt.
  • Heap constructors now take an Ordering as a constructor argument rather than a type parameter, because in general we can't get an instance from just the type (exception follows)
  • Min/max type aliases are now const BinaryMinHeap{T} = BinaryHeap{T, typeof(Forward)} and const BinaryMaxHeap{T} = BinaryHeap{T, typeof(Reverse)}, and similarly for mutable version
    • These required adding additional constructors to BinaryHeap/MutableBinaryHeap for these two special cases where the order is a singleton: BinaryHeap{T, typeof(Forward)}(...) = BinaryHeap{T}(Forward, ...) etc
  • BinaryHeap, MutableBinaryHeap, nsmallest, and nlargest now have methods with the lt, by, and rev keyword arguments like in sort, which is more user-friendly than requiring an Ordering instance.

This introduces similar breaking changes as in #547 in that isless is now used for comparison by default instead of <, which may yield different results where NaNs are involved (although, I would say that the previous behavior was undefined).
I haven't updated or run the benchmark script yet so I don't know if there are any performance regressions.

@oxinabox
Copy link
Member

oxinabox commented Aug 30, 2020

Thanks for the PR.
As you correctly identified this is redundant now that we have merged #547

@oxinabox oxinabox closed this Aug 30, 2020
@jlumpe
Copy link
Contributor Author

jlumpe commented Aug 31, 2020

I think the one thing not included is the ability to use custom orderings which are not singletons. I can create a new PR for that.

@jlumpe
Copy link
Contributor Author

jlumpe commented Sep 8, 2020

@oxinabox new PR is here: #673

@jlumpe jlumpe deleted the heaps-ordering branch November 15, 2020 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants