lspitzner / pqueue

Haskell priority queue package
Other
15 stars 13 forks source link

Imbalanced mapMaybe/mapEither APIs #114

Closed treeowl closed 1 year ago

treeowl commented 1 year ago

For MinQueue, we have

mapMaybe :: Ord k => (j -> Maybe k) -> MinQueue j -> MinQueue k

For MinPQueue, we have

mapMaybeWithKey :: Ord k => (k -> a -> Maybe b) -> MinPQueue k a -> MinPQueue k b

That is, for MinPQueue, we don't get to change/replace the keys! This may offer a better explanation for why mapMaybe and mapEither for MinQueue were wrong—they may have been "translated" from MinPQueue versions that aren't at all equivalent. But now that we're fixing the MinQueue versions (by making them much simpler), that imbalance is starting to look weird. We could add something like

mapMaybeWithKeyBlah :: Ord k => (j -> a -> Maybe (k, b)) -> MinPQueue j a -> MinPQueue k b

I don't know what you'd call that though. I suppose users could write

mapMaybeWithKeyBlah f = fromList . Maybe.mapMaybe (uncurry f) . toListU

Currently, that would be slower, because we don't have RULES for toListU for Prio queues (though we do for plain ones), but we should fix that. However, I wouldn't say it's likely terribly obvious to users that that's the right way to accomplish it.

For partitionEithers, the situation is worse: we don't currently offer anything that would give great performance implementing that in userland. I've been wanting to expose plain (untopped) queues for a while, which would open up that possibility.

konsumlamm commented 1 year ago

I don't expect such functions to be very useful. Using toListU/fromList is pretty obvious IMO, it's the first thing I would try (especially since such a function necessarily rebuilds the queue). I generally don't like such functions that just rebuild the data structure, since I think it's unneccessary bloat and may lead users to expect that it's more efficient.

I think it's weirder that mapMaybe for MinQueues exists in the first place, e.g. Sets don't have such a function either.

treeowl commented 1 year ago

All right, I guess we can just leave it as a legacy oddity.