New modules PQueue and PQueue.Prio where order type is a type parameter #14
Conversation
It is based on the implementation of MaxPQueue but uses new data type (Wrap top) instead of Down. PQueue.Top: generalization of Down data type. Bump version to 1.3.3
…nt using Top.switch
PQueue.Prio.PQ: export data constructor temporarily It is based on the implementation of MaxQueue but uses Wrap instead of Down.
PQueue.mapMaybe: use it here
…m PQueue.Prio This way, we do no longer need to expose the PQ constructor.
|
Ok I have considered this PR and its design. I have not fully understood all details but I can see how this proposal is a generalization of the existing Min/Max duplication and it matches what I had in mind after the discussion in the issue. However I have a number of reservations. Most importantly, when I said
I meant that users should not be disturbed. No phase-outs, no deprecation. With this constraint, this PR in its current form increases code duplication. On a related note, the Min/Max duplication primarily is duplication of the interface, and a generalization of the interface does not remove the complexity, but merely moves the burden to the consumer of the interface. In haskell there exist a good amount of general-purpose generalizations where this makes sense, but the generalization used here seems rather purpose-specific. As a consequence, I give the code-duplication argument a rather low subjective score, which conversely means that I am not completely opposed to adding this generic interface in addition to the existing Min/Max interfaces. I also see the possibility for turning the Max interface into a completely shallow wrapper around For more formal matters, and this confused me somewhat: I thought your main purpose of (re)adding the test-suite was to test this code, yet you opened this PR without adapting the suite to cover the newly introduced code in any way. Am I missing something? Regarding the new constructs (mainly the
|
|
On Thu, 16 Mar 2017, Lennart Spitzner wrote:
Most importantly, when I said
If this is correct and existing users won't be disturbed by such a change, I'll happily merge a PR.
I meant that users should not be disturbed. No phase-outs, no deprecation.
I also see the possibility for turning the Max interface into a
completely shallow wrapper around Queue Max, even when this is probably
less of an improvement than what you had in mind.
I considered making Prio.Max a newtype wrapper around (PQueue Max), but
that seemed to be more work and even more possibilities to make errors and
you would still have duplication of Haddock comments. Then I thought, that
the module might become superfluous anyway.
My original idea was to implement the generalization at the core. But that
required invasive changes and more complicated types. Thus I stopped that
attempt.
As a consequence, I give the code-duplication argument a rather low
subjective score, which conversely means that I am not completely
opposed to adding this generic interface in addition to the existing
Min/Max interfaces.
My proposed code does not save code at the library side, and it cannot,
given that the existing interface shall be preserved and that it would
require at least a substantial amount of wrapper code. However, it saves
code on the client side. Every utility function in client code can now be
defined uniformly for Max or Min.
If you want to avoid code duplication in the library, too, we could use a
generic wrapper module that we duplicate using CPP or some semi-automatic
process. We could add this in a further step.
For more formal matters, and this confused me somewhat: I thought your
main purpose of (re)adding the test-suite was to test this code, yet you
opened this PR without adapting the suite to cover the newly introduced
code in any way.
It's not complete, yes. My confidence so far is entirely based on the fact
that I did only search&replace on the Max modules.
Regarding the new constructs (mainly the Top module):
* Some comments on the newly introduced constructs would be helpful. (Especially the type class - I generally
dislike classes that lack any laws or explanations.)
Ok. The only law is that the class can only have Min or Max as instances.
Users cannot add more instances (unless they use "undefined"). There are
no more laws. That's what I call a closed-world class.
* The names confused me somewhat: "Top" feels semantically close to "maximum", which makes it a bad choice for
a thing that can me Max or Min. "First" would be a better choice imo. And "Wrap" is very close to the Tagged
type - it differs only in the Ord-instance, as far as I can tell. I think this fact deserves a comment too,
if not a rename.
It depends on whether you imagine vertical or horizontal queues. I am
happy with a horizontal queue, too. Unfortunately Git does not support
renaming identifiers like Darcs does.
* I am mildly certain that the newtypes (Compare, ToList, …) can be avoided. I won't discuss this until the
more important issues are resolved.
These newtypes are required by the closed-world approach. The idea of the
closed-world class is that you cannot add more instances, but in return
you can add any number of "methods" using these helper newtypes. The class
API does not change by adding such "methods" (interesting for PVP
compliance) and even a client can add "methods" without the need to define
sub-classes.
|
But you implied that it did when you said
|
|
On Thu, 16 Mar 2017, Lennart Spitzner wrote:
My proposed code does not save code at the library side, and it cannot, given that the existing
interface shall be preserved and that it would require at least a substantial amount of wrapper
code.
But you implied that it did when you said
It will save you code and Haddock duplication.
(As we have seen, it is pretty easy to get the duplicated Haddock comments wrong.)
I wrote that with phasing out Min/Max modules in mind. If you do not want
that way, we will necessarily have code duplication.
|
|
Ok. I guess I merely find it confusing that you make a (apparently new) code-duplication argument in response to what was said before. |
I don't see how "first" is horizontal (?) And is Darcs vs Git relevant at all for this discussion? |
|
On Thu, 16 Mar 2017, Lennart Spitzner wrote:
It depends on whether you imagine vertical or horizontal queues. I am happy with a horizontal queue,
too. Unfortunately Git does not support renaming identifiers like Darcs does.
I don't see how "first" is horizontal (?)
You are right. That would have been "left".
And is Darcs vs Git relevant at all for this discussion?
In Darcs it would be simpler to apply your proposed renaming afterwards.
:-)
|
|
Which remains irrelevant. Sorry. |
|
Ok, back to more constructive feedback: Let me explain what I have in mind to avoid the newtypes, one sec. |
|
On Thu, 16 Mar 2017, Lennart Spitzner wrote:
Ok. I guess I merely find it confusing that you make a (apparently new)
code-duplication argument in response to what was said before.
When I opened the ticket, I hoped for reduction of code duplication
because I expected that we can have one core generic module and make Min
and Max thin wrappers around it. Then I found there is no test-suite that
could check my modifications. Then I found a test-suite, but it lacked to
check all modules and all functions. But I added it, because it is better
than nothing. Then I found that it is not at all easy to generalize the
core implementation and decided to keep the chain Internals->Min->Max, but
copy Max to Top. Now, code un-duplication can only occur, if we phase out
Max. Now, since this is not wanted, too, we can only have code savings on
the client side.
|
|
I understand, and I can see that the whole process may be frustrating for you. However I still think that I have properly communicated the constraint that I would put on this PR early on. I'll say this again to make it clear: I am not opposed to merging this on the basis that it increases duplication; I was actually arguing for duplication in my initial reply to this PR. Am I assuming correctly that you still want this merged, even when we do not deprecate the existing interface? |
|
On Thu, 16 Mar 2017, Lennart Spitzner wrote:
I understand, and I can see that the whole process may be frustrating
for you. However I still think that I have properly communicated the
constraint that I would put on this PR early on.
You said:
"If this is correct and existing users won't be disturbed by such a change,"
I assumed that by no longer adding public functions to Min/Max you would
not disturb existing users. You can make the other phases of deprecation
as long as you want. :-)
Am I assuming correctly that you still want this merged, even when we do
not deprecate the existing interface?
Right.
|
Ok, fair :-) Then we are on the same page. |
|
When defining class First first where
switch :: a -> a -> TaggedF first a
instance First Min where switch f _ = TaggedF f
instance First Max where switch _ f = TaggedF fwe can avoid the instance (First first, Ord a) => Ord (TaggedF first a) where
compare x y = unTaggedF $ switch compare (flip compare) <*> x <*> yAnd by defining unSwitchQueue
:: (Min.MinQueue (TaggedF first a) -> TaggedF first b) -> Queue first a -> b
unSwitchQueue f (Q q) = unTaggedF (f q)we can get rid of foldrDesc f z = unSwitchQueue $ \q ->
switch (Min.foldrDesc (f . unTaggedF) z q) (Min.foldrAsc (f . unTaggedF) z q)I haven't tested this either, but it should have the same semantics; it at least compiles so far. Or is there some inherent disadvantage to this approach which I miss? Using Applicative or appropriate general-purpose helpers (unswitch) seem to serve the purpose of "connecting the phantom types" just as well as the newtypes. |
|
( |
|
I would have committed the changes I made already, but they don't compile yet. I have pushed the not-yet-compiling code to my generic-top branch. |
|
On Thu, 16 Mar 2017, Lennart Spitzner wrote:
When defining Top First like this:
class First first where
switch :: a -> a -> TaggedF first a
instance First Min where switch f _ = TaggedF f
instance First Max where switch _ f = TaggedF f
we can avoid the Compare newtype like this:
instance (First first, Ord a) => Ord (TaggedF first a) where
compare x y = unTaggedF $ switch compare (flip compare) <*> x <*> y
And by defining
unSwitchQueue
:: (Min.MinQueue (TaggedF first a) -> TaggedF first b) -> Queue first a -> b
unSwitchQueue f (Q q) = unTaggedF (f q)
we can get rid of Foldr, Foldl, ToList and write, e.g.
Does your approach also work for functions with TaggedF as result, e.g.
FromList? Maybe it works and you need an extra unSwitchQueue instead of an
extra newtype for every new type signature?
That said, we could get rid of Foldr, Foldl and ToList by using a unified
newtype wrapper, say,
newtype Fold b a first = Fold {runFold :: Queue first a -> b}
and adapt foldr, foldl and toList to match this interface. We would save
two newtype definitions and get in return a bit of adaption code that must
be duplicated for the Min and the Max branch in 'switch'. You already
write this duplicated adaption code (here: unTaggedF) in your example to
match the generic unSwitchQueue:
… foldrDesc f z = unSwitchQueue $ \q ->
switch (Min.foldrDesc (f . unTaggedF) z q) (Min.foldrAsc (f . unTaggedF) z q)
|
|
On Thu, 16 Mar 2017, Lennart Spitzner wrote:
I would have committed the changes I made already, but they don't
compile yet. I have pushed the not-yet-compiling code to my generic-top
branch.
I think that your approach and my one so far are pretty equivalent. I can
convert your 'compare' implementation and your 'unSwitchQueueConstr' to my
newtype style and you can express with functions what I express with
types. I use the newtypes to select the type to switch on, and you use
functions for that. I have to think a bit more about it, maybe your
approach using functions is more flexible. That would be cool and would be
useful for other packages where I use closed-world classes, e.g. pathtype.
|
|
True, this still requires 2 helper functions in place of the 5 newtypes (I don't count unTaggedF as a helper function; the newtype-approach requires |
|
Btw, can we merge the Private and Internals modules? |
|
On Thu, 16 Mar 2017, Lennart Spitzner wrote:
True, this still requires 2 helper functions in place of the 5 newtypes
As I said, I can convert your implementations to the newtype style, and
then there remain only 2 newtypes, one corresponding to unSwitchQueue and
one corresponding to unSwitchQueueConstr.
|
|
On Thu, 16 Mar 2017, Lennart Spitzner wrote:
Btw, can we merge the Private and Internals modules?
Prio.Internals is the private module for Prio.Min and Prio.Private is the
private module for Prio. They could have better names, but I would not
merge them.
|
Right. the difference is indeed fairly small. Both this suggestion and the question of how to name things don't hold back this PR. I'd like proper testing and some comments (and I was just as guilty when not adding my reasoning for |
Good point. Also deserves a comment at the top of these two modules :-) |
|
On Thu, 16 Mar 2017, Lennart Spitzner wrote:
Prio.Internals is the private module for Prio.Min and Prio.Private is the private module for Prio.
They could have better names, but I would not merge them.
Good point. Also deserves a comment at the top of these two modules :-)
I'd prefer better module names but hesitated to rename them so far,
because that would be more invasive and erm, Git does not directly support
moving files, in contrast to Darcs. :-)
|
|
haha :-) |
|
On Thu, 16 Mar 2017, Lennart Spitzner wrote:
As I said, I can convert your implementations to the newtype style, and then there remain only 2
newtypes, one corresponding to unSwitchQueue and one corresponding to unSwitchQueueConstr.
Right. the difference is indeed fairly small.
There is one small difference: My approach asserts that the two instances
of Top are correct and not swapped.
I tried to prove that both approaches are equivalent. I assume they are
equivalent in our case, where Min and Max are phantoms. But I am afraid
that I cannot generalize it to my other projects.
Here is my attempt to prove equivalence:
~~~~
module ClosedWorld where
data Min = Min
data Max = Max
class Top top where
switchTop :: f Min -> f Max -> f top
instance Top Min where switchTop f _ = f
instance Top Max where switchTop _ f = f
switchTopFromFirst :: (First top) => f Min -> f Max -> f top
switchTopFromFirst = undefined switchFirst
newtype Tagged first a = Tagged {untag :: a}
class First first where
switchFirst :: a -> a -> Tagged first a
instance First Min where switchFirst a _ = Tagged a
instance First Max where switchFirst _ a = Tagged a
newtype Const a first = Const a
taggedFromConst :: Const a first -> Tagged first a
taggedFromConst (Const x) = Tagged x
switchFirstFromTop :: (Top first) => a -> a -> Tagged first a
switchFirstFromTop x y = taggedFromConst $ switchTop (Const x) (Const y)
~~~~
We would need to implement switchTopFromFirst with the given type
signature using only switchFirst.
|
|
I agree, in general Now I wonder if one can avoid newtypes even with that.. I have to play with this a bit. (And no, newtypes are not evil and this is probably not very important.) |
|
I've been skimming comments, but ... I'm so far failing to understand the point of any of this. What's wrong with just turning a concrete min-priority queue into a max-priority queue using |
This is finally my proposed code for solving #8.
I defined the type Wrap that is like the Down datatype
but it has a 'top' type parameter to choose
whether minimum or maximum should be at the top of the queue.
The new modules PQueue and PQueue.Prio are rather copies
of their Max counterparts, using Wrap instead of Down.
I like that approach very much! :-)
Thus I'd propose to phase out public PQueue.Min/Max and PQueue.Prio.Min/Max modules slowly,
that is, add new functions only to PQueue and PQueue.Prio,
and somewhen deprecate PQueue.Min/Max and PQueue.Prio.Min/Max,
and very far in the future remove Max modules and Down and make Min modules private.
It will save you code and Haddock duplication.
(As we have seen, it is pretty easy to get the duplicated Haddock comments wrong.)