Better hasEdge for Algrebra.Graph

[nobrakal]

Hi,

I have played again a bit, but I think I have found a better one this time :)
The boost come from two things:

1. The first one, I had never checked it, but induce (\x -> elem x [u,v]) was not a great idea. Just replacing it by induce (\x -> x == u || x == v) is clearly a better idea:

Clique

	1000
WithEq	70.80 ms
WithElem	103.4 ms

2. Then, because the equality test was already done in the "induce", I had the idea to store it and pass it to a modified version from ToGraph:

hasEdge :: Eq a => a -> a -> Graph a -> Bool
hasEdge s t = hasEdge' . induce'
    where
     hasEdge' g = case foldg e v o c g of (_, _, r) -> r
       where
         e                             = (False   , False   , False                 )
         v x                           = (x       , not x   , False                 )
         o (xs, xt, xst) (ys, yt, yst) = (xs || ys, xt || yt,             xst || yst)
         c (xs, xt, xst) (ys, yt, yst) = (xs || ys, xt || yt, xs && yt || xst || yst)
     induce' = foldg Empty
                    (\x -> if x == s then Vertex True else if x == t then Vertex False else Empty)
                    (k Overlay)
                    (k Connect)
       where
         k _ x     Empty = x -- Constant folding to get rid of Empty leaves
         k _ Empty y     = y
         k f x     y     = f x y

It is a bit more complicated, but it drops the Ord instance previously required to an Eq one:

Mesh

	1000
MoreComplicated	112.9 μs
AlgaOld	219.7 μs

Clique

	1000
MoreComplicated	59.75 ms
AlgaOld	90.81 ms

SUMMARY:

• Alga was the fastest 12 times

ABSTRACT:
(Based on an average of the ratio between largest benchmarks)

• Alga was 1.71 times faster than AlgaOld

And this rocks also with alga's clique:

Alga's Clique

	1000
MoreComplicated	20.09 μs
AlgaOld	51.48 μs

[nobrakal]

This is obviously related to #84

[nobrakal]

Well it worked, but only when dealing with non-loop :p

The corrected version is passing tests and is a bit slower than the faulty one of course.

Mesh

	1000
Working	126.0 μs
AlgaOld	230.8 μs

Clique

	1000
Working	69.36 ms
AlgaOld	97.86 ms

[nobrakal]

Bang patterns wasn't useful and does not worked with old GHCs. Thanks travis !

Mesh

	1000
Alga	109.8 μs
AlgaOld	224.9 μs

Clique

	1000
Alga	60.98 ms
AlgaOld	94.53 ms

[snowleopard]

@nobrakal What about doing a simple check at the beginning: if x == y then check for a self-loop, otherwise, you a simplified version of hasEdge for definitely different vertices?

[nobrakal]

What about doing a simple check at the beginning: if x == y then check for a self-loop,

I don't know if it worth it: the version with tuples (the last one) is almost the same time than my first faulty version, and is working in any cases. It must be a little worse in term of space use (I am storing two booleans instead of one for matching vertex), but I don't think this is an issue ^^.
As we are offering now a hasSelfLoop, if a user now he is searching for a loop, it can definitely use it.

[snowleopard]

@nobrakal I am concerned with the complexity of the current solution: it is hard to understand and it's not immediately obvious that it is correct.

By checking for self-loops separately, we can potentially get two benefits:

• Simpler code and intuition that we can explain in a comment: we convert a Graph a to a Graph Bool and then look for an edge from False to True.

• Potentially better performance in case of self-loops and also in case of non-self-loops, since you no longer need to carry around the tuples in Vertex. But this needs to be benchmarked.

[snowleopard]

hasEdge: 0.26 (good)

@nobrakal Wow, is it 4 times faster now?

[nobrakal]

@nobrakal Wow, is it 4 times faster now?

It seems yep ^^. Maybe this is a bit overestimated. I have ran the benchs on my machine with bigger graphs, and it is more about 0.40

Just updated this PR using your suggestion. It worth anyway to test the loop presence more than I thought :)

Here:

• Alga is the version where we test for a loop
• AlgaOld is the version with a tuple of Booleans

Mesh

	1000
Alga	98.31 μs
AlgaOld	143.2 μs

Clique

	1000
Alga	75.22 ms
AlgaOld	84.08 ms

SUMMARY:

• Alga was the fastest 8 times

There was 4 ex-aequo

ABSTRACT:
(Based on an average of the ratio between largest benchmarks)

• Alga was 1.26 times faster than AlgaOld

Please note that the benchs were run on another machine than the previous one, so I think trying to compare them with my previous comments does not make many sense.

[snowleopard]

@nobrakal I have another (very low-level) optimisation in mind, but I think we can try it separately :)

Could you do the same change to NonEmpty?

Can this be used in ToGraph, which I guess needs hasSelfLoop as well?

[nobrakal]

@nobrakal I have another (very low-level) optimisation in mind, but I think we can try it separately :)

Haha, you awaken my curiosity ^^

Could you do the same change to NonEmpty?

I think I have already done it ^^: 9111701

Can this be used in ToGraph, which I guess needs hasSelfLoop as well?

It has hasSelfLoop = hasEdge x x

I think it heavily depends on the removing of empty leaves by induce...

[nobrakal]

Oops you were right, I just added the corresponding change for Algebra.Graph.NonEmpty

[snowleopard]

@nobrakal Thanks! Does this optimisation make sense for Algebra.Graph.Fold?

[snowleopard]

@nobrakal Looks like there are now some conflicts.

[nobrakal]

Using the still-hot NFData instance of Fold, I found that the optimization makes sense for it :)
The benchs, Alga being the state of this PR and AlgaOld the current state of the master branch

hasEdge

Description: Test if the given edge is in the graph (with arguments both in the graph and not in the graph (where applicable))

Clique

	1000
Alga	70.79 ms
AlgaOld	797.4 ms

Mesh

	1000
Alga	262.8 μs
AlgaOld	906.3 μs

SUMMARY:

• Alga was the fastest 12 times

ABSTRACT:
(Based on an average of the ratio between largest benchmarks)

• Alga was 5.27 times faster than AlgaOld

hasSelfLoop

Description: Test if the given self-loop is in the graph (with arguments both in the graph and not in the graph (where applicable))

Clique

	1000
Alga	56.15 ms
AlgaOld	780.0 ms

Mesh

	1000
Alga	183.1 μs
AlgaOld	896.9 μs

SUMMARY:

• Alga was the fastest 8 times

ABSTRACT:
(Based on an average of the ratio between largest benchmarks)

• Alga was 7.24 times faster than AlgaOld

[snowleopard]

@nobrakal Great! But what happened with Travis non-empty instance?

https://travis-ci.org/snowleopard/alga/jobs/405842227

[nobrakal]

My bad, my script is very sensitive (maybe too much) to any change in then benchmark suite.
It is updated and should work, sorry !

[snowleopard]

@nobrakal Why do we no longer have changes to non-empty graphs? Did your commit get lost?

[nobrakal]

I totally forget about this PR while doing #94 so I blindly added the optimization to it.

So it is already in place for Algebra.Graph.NonEmpty

[snowleopard]

@nobrakal Aha, I see! Thanks, merged :)