-
Notifications
You must be signed in to change notification settings - Fork 12
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? |
konsumlamm
left a comment
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?
|
|
||
| -- | 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
EqandOrdinstances forPrioqueues 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
Prioqueues.