Add bifunctor instance for labelled graphs

[andreabedini]

I was looking for simple contributions to give to an Haskell project and I found #156, so I have given it a go. This is my first multi character contribution in Haskell so any kind of comments or suggestions are welcome :-)

Also let me know if this corresponds to what you had in mind.

[nobrakal]

Hi there,

Happy to see you here :D

Your implementation seems totally correct, but leads to an unneeded destruction/reconstruction of the graph (fmap will first deconstruct the graph, change the vertices, then reconstruct it, while emap will do the same thing changing the label of the edges).

I am pretty sure this can be avoided using directly foldg to go through the graph structure only once.

Otherwise, I don't see any problem :)

[snowleopard]

Hi @andreabedini! Thank you for the PR!

I agree with @nobrakal, your implementation can be made more efficient via foldg:

    bimap f g = foldg Empty (Vertex . g) (Connect . f)

I hope my suggestion compiles and passes tests, I haven't actually tried it :)

[snowleopard]

Actually, speaking of tests: could you add a test that actually uses your implementation, i.e. tests that for any functions f and g, and any labelled graph x, we have bimap f g x == (emap f . fmap g) x?

Have a look at how we test emap:

https://github.com/snowleopard/alga/blob/master/test/Algebra/Graph/Test/Labelled/Graph.hs#L346-L377

Our current tests are not ideal, as you can see in the TODO.

[andreabedini]

@nobrakal @snowleopard thanks for the feedback (and for the warm welcome!). I have reimplemented bimap as @snowleopard suggested, also defined emap in terms of bimap where we have it. Tests seems to pass.

@snowleopard I'll have a look at how to improve the tests a bit later.

By the way, this got me thinking. Is there an expectation for Functor and Bifunctor instances to be "compatible"? I.e. for a type F a b where F is a Bifunctor and F a is a Functor. Should one expect that fmap f . bimap g h == bimap g (f . h)? said differently fmap == second?

[snowleopard]

I have reimplemented bimap as @snowleopard suggested, also defined emap in terms of bimap where we have it. Tests seems to pass.

@andreabedini Nice! Well, it was expected, but it's good to have it tested :) I would however prefer to revert to the previous definition of emap since it's less noisy.

Should one expect that fmap f . bimap g h == bimap g (f . h)?

Actually, I think this is a free theorem, i.e. it's impossible to violate this law given that bimap id id == id. You can find some intuition here: https://github.com/quchen/articles/blob/master/second_functor_law.md

[andreabedini]

@andreabedini Nice! Well, it was expected, but it's good to have it tested :) I would however prefer to revert to the previous definition of emap since it's less noisy.

Done!

Actually, I think this is a free theorem, i.e. it's impossible to violate this law given that bimap id id == id. You can find some intuition here: https://github.com/quchen/articles/blob/master/second_functor_law.md

Right, I was thinking only in terms of laws but seems to be a consequence of parametricity. I appreciate the link, thanks.

[snowleopard]

@andreabedini Thank you!

I think we can merge this as is, without Bifunctor tests, because at the moment we have generally poor test coverage for many instances, which should be addressed in #120.

Let me know what you prefer: merge as is, or wait until you implement some tests. I'm happy either way!

[andreabedini]

Let me know what you prefer: merge as is, or wait until you implement some tests. I'm happy either way!

I'd say given we have a ticket already opened for better test coverage and you are ok with this, we can merge this first (so it doesn't get hung up on me writing more tests). I have started on #150 as well and I might finish that before looking at the tests.

[snowleopard]

@andreabedini Sounds great, merged! Thank you for this and future PRs :-)