Use Semigroup constraint for LAM instead of Monoid

[dwincort]

As discussed in #307, this eases the Monoid restriction labeled adjacency maps to Semigroup.

[dwincort]

One function that still has a Monoid constraint is edgeLabel. I think, really, this should have the type edgeLabel :: (Semigroup e, Ord a) => a -> a -> AdjacencyMap e a -> Maybe e, where Nothing represents the lack of an edge. However, I didn't want to change the interface to the module, so I just left it alone. Let me know if you think I should change it.

I think I've updated most of the comments that mention zero or Monoid from Labelled.AdjacencyMap, but I haven't really added any new documentation (outside of the comment on trimZeroes.

[dwincort]

This is a little frustrating but probably acceptable. Technically, LAM.AdjacencyMap seems like it could be a Graph so long as it has the multiplicative monoid even if it doesn't have the additive one (that is, it has <.> and one but doesn't have zero). Thus, the Dioid e constraint is too strong. However, there's no nice way to represent this, so it seems like we should just require the full Dioid e constraint.

[dwincort]

I haven't used this module much, but I'm pretty sure this is the right place to make a minimal change that will appropriately trim zero out of the adjacency maps produced from this module.

[dwincort]

Exposing this seems like an abstraction leak. However, keeping it hidden could cause users to suffer from performance penalties if their edge type has zero.

[snowleopard]

Given that the implementation doesn't care much about zeroes, should we provide a more general function like filterEdges?

[dwincort]

I want to propose a radical alternative. Instead of exposing trimZeros (or filterEdges), we make the monoid explicit and this function implicit. I feel like Haskell's type classes are not quite equipped to deal with this, but I can think of a work around. We could define:

data AdjacencyMap e a = AM {
    -- | The /adjacency map/ of an edge-labelled graph: each vertex is
    -- associated with a map from its direct successors to the corresponding
    -- edge labels.
    adjacencyMap :: Map a (Map a e),
    zeroEdge :: Maybe e
  } deriving (Eq, Generic, NFData)

Then, we could have two constructors, one that takes a Semigroup constraint and sets zeroEdge = Nothing and the other that takes a Monoid constraint and sets it to mempty. We'd add back in all the uses of filterZeros, but they'd be guarded by whether zeroEdge was Nothing or not.

[dwincort]

Oh, also, I left consistent :: (Ord a, Monoid e, Eq e) => AdjacencyMap e a -> Bool with a Monoid constraint. It feels like it could be useful for AdjacencyMaps where e is a Monoid.

[snowleopard]

Overlaying with empty doesn't seem to make much sense here. I think I would actually prefer to keep this comment talking about <+> and zero, for symmetry with the one below, saying something like "Assuming that 'zero' exists...".

[snowleopard]

Also, if you're changing examples here, you should update them in the testsuite too.

[snowleopard]

Thanks! Overall, the PR looks good but I left a couple of comments.

[snowleopard]

I don't like that this module becomes so different from Algebra.Graph.Labelled but not sure what to do about this. Something doesn't feel right, still.

[dwincort]

I agree. Something still feels off.

I think the weirdness comes down to the fact that the edge type, really, is a monoid, even if it's only declared as a semigroup. This is because any semigroup can be trivially lifted to a monoid by wrapping it with Maybe, or, more generally, by defining a new, separate empty value and properly updating the binary operator to treat it appropriately. In the case of these adjacency maps, we're doing exactly this: we're treating the absence of an edge as mempty, thus forming an ad hoc monoid.

So, I think the reason the above text feels off is that, morally, we're constructing AdjacencyMap (Maybe ()) a as isomorphic to unlabelled graphs, but we're totally hiding the Maybe part as an implementation detail. (To be clear, this "hiding Maybe as an implementation detail" is the whole reason I proposed this change in the first place.) This should probably be called out in this comment.

[dwincort]

FYI, the project I was working on that used this got scrapped, so while I'm still happy to help make it better and/or brainstorm ideas, I wouldn't even be a user if it got merged. In other words, I won't be offended if you want to drop this PR entirely.