-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Benchmark hasEdge #84
Comments
Interesting, thanks! I don't fully understand what you benchmarked, can you show the definition you used for the Could it be that we should specialise |
Sorry if I wasn't clear. For the FIRST table, I used (copy/pasted from ToGraph) hasEdge s t g = case foldg e v o c g of (_, _, r) -> r
where
e = (False , False , False )
v x = (x == s , x == t , 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) For the SECOND, I used hasEdge :: Ord a => a -> a -> Graph a -> Bool
hasEdge s t = (\g -> case foldg e v o c g of (_, _, r) -> r) . induce (`elem` [s, t])
where
e = (False , False , False )
v x = (x == s , x == t , 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) |
Wow, that's pretty surprising to me. So, essentially, on What about making the third field (or maybe all of them) of the tuple strict? Perhaps, we are accumulating huge unevaluated trees of Boolean expressions in the process? |
One simple tweak could probably be as follows: hasEdge s t g = case foldg e v o c g of (_, _, r) -> r
where
e = (False , False , False)
v x = (x == s , x == t , False)
o (xs, xt, xst) (ys, yt, yst) = (xs || ys, xt || yt, xst || yst)
c (_, _, True) (_, _, _) = (True, True, True)
c (_, _, _) (_, _, True) = (True, True, True)
c (xs, xt, xst) (ys, yt, yst) = (xs || ys, xt || yt, xs && yt) This essentially forces the third field for the |
Sorry for taking so long to answer. Your implementation is faster but not enough: Clique
Note that this is certainly the fault of Clique
Anyway, one advantage of the |
I don't understand what you mean: both algorithms are working on exactly the same input, right? It's not the fault of the input that one algorithm takes 7x longer than another :) |
Mmh, yes your are right, I wanted to say to we must not forget that we are only using a very big stack of overlayed edges from |
I spent some time trying to improve your implementation, and I think that it is maybe not worth spending time here, the actual implementation is faster everywhere (on real graphs made using The only improvement that can be made I think is something like: hasEdge :: Eq a => a -> a -> Graph a -> Bool
hasEdge u v = hasEdge' . induce (`elem` [u, v])
where
hasEdge' (Overlay x y) = hasEdge' x || hasEdge' y
hasEdge' (Connect x y) = let !isInLeft = hasVertex u x
!isInRight = hasVertex v y
in (isInLeft && isInRight) || (isInLeft && hasEdge' x) || (hasEdge' y && isInRight)
hasEdge' _ = False Because it drop the It can also be great to define hasLoop x = hasLoop' . induce (==x)
where
hasLoop' (Overlay x y) = hasLoop' x || hasloop' y
hasLoop' Connect{} = True
hasLoop' _ = False Which I think is working because |
@nobrakal I don't think your |
Yep totally, I was thinking with |
@nobrakal Right, I see. I don't mind adding So, what's the conclusion regarding |
You don't mind to do it yourself but will merge if I do it ? I prefer to ask ^^
I was also ^^. The actual one is the fastest, with graphs constructed with hasEdge :: Eq a => a -> a -> Graph a -> Bool hasEdge u v = hasEdge' . induce (`elem` [u, v])
where
hasEdge' (Overlay x y) = hasEdge' x || hasEdge' y
hasEdge' (Connect x y) =
let !isInLeft = hasVertex u x
!isInRight = hasVertex v y
in (isInLeft && isInRight) || (isInLeft && hasEdge' x) || (hasEdge' y && isInRight)
hasEdge' _ = False This improve a bit the time of the function (it is about |
I don't mind either, but I won't find time to do it myself in the next couple of weeks, so feel free to do it :-) OK, let's switch to your improved implementation -- it's not immediately obvious it is correct but it actually looks quite nice. Please send a PR! |
I was going to send a PR, and while verifying my results, I benchmarked to hasEdge :: Eq a => a -> a -> Graph a -> Bool
hasEdge u v = hasEdge'
where
hasEdge' (Overlay x y) = hasEdge' x || hasEdge' y
hasEdge' (Connect x y) =
let !isInLeft = hasVertex u x
!isInRight = hasVertex v y
in (isInLeft && isInRight) || (isInLeft && hasEdge' x) || (isInRight && hasEdge' y)
hasEdge' _ = False So without the suffixed
The first three tested edges are edges in the graph, the last three are not in the graph. Sadly this is "less" true for graph constructed using
|
Wow, what happens here?
It's 100 times slower here! Could you investigate why? |
The clique is in the form That is why it works great with #80 representation ^^ |
Ah, of course -- the version that calls |
I tried to do some memoization, but nothing great :p So do you think we can approach the time of the version using Or even propose the two versions ? The |
Well, you are comparing an O(n) implementation against an O(n^2) one. Of course the latter will always lose on large benchmarks.
I don't think there is any point in including a quadratic version into the library when there is a linear implementation. Think why the quadratic version manages to be faster on some examples -- perhaps that will help you figure out how to improve the linear version too! |
It runs I spent again some time trying to find a better version, but didn't find anything, so maybe just dropping the |
@nobrakal Sorry, I'm not sure which implementation you refer to. Perhaps, just create a PR and we'll discuss it there :) |
I think this is closed with your super version of |
@nobrakal Yes, thank you. I think we can close this. |
Hi,
This is just for the record:
I saw: https://github.com/snowleopard/alga/blob/master/src/Algebra/Graph.hs#L449 :
So here is the benchmarks:
Alga
use the the ToGraph definition,AlgaOld
is the current implementation:hasEdge
Description: Test if the given edge is in the graph (with arguments both in the graph and not in the graph (where applicable))
Mesh
Clique
SUMMARY:
ABSTRACT:
(Based on an average of the ratio between largest benchmarks)
As the ToGraph option is faster on small graphs, I tried a combination of the two:
Which sadly is not better:
hasEdge
Description: Test if the given edge is in the graph (with arguments both in the graph and not in the graph (where applicable))
Mesh
Clique
SUMMARY:
There was 13 ex-aequo
ABSTRACT:
(Based on an average of the ratio between largest benchmarks)
The text was updated successfully, but these errors were encountered: