-
Notifications
You must be signed in to change notification settings - Fork 13
Eq and Ord instances for Prio queues #106
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
base: master
Are you sure you want to change the base?
Conversation
* Make the `Eq` and `Ord` instances for `Prio` queues compare the queues in fully sorted form—that is, as key-value maps. This seems to be the only way to make these instances make any real kind of sense. * Document the "nondeterministic" nature of `Prio` queues.
@konsumlamm Any comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't have much time lately, I'll try to keep up with your PRs.
I'm a bit skeptical about requiring Ord a
for Eq (MinPQueue k a)
. That would be a breaking change and many value types I can imagine don't necessarily have an Ord
instance (although I can't think of many use cases for an Eq
instance anyway...). I'd be fine with just documenting that the Eq
instance is non-deterministic and recommending (==) `on` toAscList
. However, then we also can't change the Ord
instance, since they should be compatible.
Btw, please try to link related issues, like #78. Is this supposed to close #78?
@@ -128,7 +128,7 @@ pattern Empty = Internals.Empty | |||
infixr 5 :< | |||
|
|||
-- | A bidirectional pattern synonym for working with the minimum view of a | |||
-- 'MinPQueue'. Using @:<@ to construct a queue performs an insertion in | |||
-- 'Prio.MinPQueue'. Using @:<@ to construct a queue performs an insertion in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have been 'MinQueue'
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll fix that.
-- | \(O(n)\). @'mapKeysMonotonic' f q == 'Data.PQueue.Prio.Min.mapKeys' f q@, | ||
-- but only works when @f@ is strictly monotonic. | ||
-- /The precondition is not checked./ | ||
-- This function has better performance than 'Data.PQueue.Prio.Min.mapKeys'. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-- | \(O(n)\). @'mapKeysMonotonic' f q == 'Data.PQueue.Prio.Min.mapKeys' f q@, | |
-- but only works when @f@ is strictly monotonic. | |
-- /The precondition is not checked./ | |
-- This function has better performance than 'Data.PQueue.Prio.Min.mapKeys'. | |
-- | \(O(n)\). @'mapKeysMonotonic' f q == 'Data.PQueue.Prio.Min.mapKeys' f q@, | |
-- but only works when @f@ is strictly monotonic. | |
-- /The precondition is not checked./ | |
-- This function has better performance than 'Data.PQueue.Prio.Min.mapKeys'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change is to remove the blank line, unfortunately the diff doesn't show this.
I really want the |
Yes this should close #78. |
Make the
Eq
andOrd
instances forPrio
queues compare the queues in fully sorted form—that is, as key-value maps. This seems to be the only way to make these instances make any real kind of sense.Document the "nondeterministic" nature of
Prio
queues.