Remove foldable #121
Remove foldable #121
Conversation
src/Algebra/Graph.hs
Outdated
|
|
||
| The 'size' of any graph is positive, and the difference @('size' g - 'length' g)@ | ||
| corresponds to the number of occurrences of 'empty' in an expression @g@. | ||
| Note that 'size' count all leaves of the expression. |
There was a problem hiding this comment.
"count" -> "counts"
Also, I think we can keep size-related examples above, and replace length examples by vertexCount examples.
src/Algebra/Graph.hs
Outdated
| -- foldg 1 (const 1) (+) (+) == 'size' | ||
| -- foldg True (const False) (&&) (&&) == 'isEmpty' | ||
| -- foldg False ((==) v) (||) (||) == 'hasVertex v' |
There was a problem hiding this comment.
Let's switch to foldg False (==x) (||) (||) == hasVertex x to match the testsuite as well as actual hasVertex implementation.
src/Algebra/Graph.hs
Outdated
| ys = map (\a -> fmap (a,) y) $ toList x | ||
| xs = map (\b -> fmap (,b) x) $ toListGr y | ||
| ys = map (\a -> fmap (a,) y) $ toListGr x | ||
| toListGr = foldg [] pure (++) (++) |
There was a problem hiding this comment.
I suggest to use the name toList since the function is equivalent to the standard toList.
By the way, are you sure this is fast enough? Do we need to worry about potential quadratic list concatenation?
You might want to use Algebra.Graph.Internal.List instead to achieve constant-time list concatenation.
There was a problem hiding this comment.
Indeed, it is better !
src/Algebra/Graph/Fold.hs
Outdated
|
|
||
| The 'size' of any graph is positive, and the difference @('size' g - 'length' g)@ | ||
| corresponds to the number of occurrences of 'empty' in an expression @g@. | ||
| Note that 'size' count all leaves of the expression. |
There was a problem hiding this comment.
src/Algebra/Graph/Fold.hs
Outdated
| -- foldg 1 (const 1) (+) (+) == 'size' | ||
| -- foldg True (const False) (&&) (&&) == 'isEmpty' | ||
| -- foldg False ((==) v) (||) (||) == 'hasVertex v' |
There was a problem hiding this comment.
| -- 'overlay' (stars xs) (stars ys) == stars (xs ++ ys) | ||
| -- @ | ||
| stars :: Graph g => [(a, [a])] -> g a | ||
| stars = overlays . map (uncurry star) |
There was a problem hiding this comment.
Does it also make sense to switch to your better overlays implementation based on foldr1Safe?
There was a problem hiding this comment.
I don't know, I think yes ! I will do some benchs tomorrow
There was a problem hiding this comment.
Mmhh, I spoke too quickly, the concatg solution seems to not work, at least not out-of-the-box...
It will certainly require further work, maybe this is linked with the same problem in Algebra.Graph.Fold
src/Algebra/Graph/Labelled.hs
Outdated
| -- foldgUnDiff True (const False) (&&) == 'isEmpty' | ||
| -- foldgUnDiff False ((==) v) (||) == 'hasVertex v' | ||
| -- @ | ||
| foldgUnDiff :: b -> (a -> b) -> (b -> b -> b) -> Graph e a -> b |
There was a problem hiding this comment.
Let's not make any changes to this module in this PR -- I've got a big refactoring of labelled graphs in a local branch where I implemented a similar fold function and removed Foldable -- I'll make the corresponding PR soon.
src/Algebra/Graph/NonEmpty.hs
Outdated
| The 'size' of any graph is positive and coincides with the result of 'length' | ||
| method of the 'Foldable' type class. We define 'size' only for the consistency | ||
| with the API of other graph representations, such as "Algebra.Graph". | ||
| Note that 'size' count all leaves of the expression. |
There was a problem hiding this comment.
|
@nobrakal Many thanks! I left some comments above, but looks good in general.
Let's not do anything about it in this PR -- I've already removed |
|
@nobrakal Is this ready or do you plan to do any more work? |
|
I think this is ready. The |
|
@nobrakal Thanks, I'll merge it in a couple of days -- want to have another look, but currently travelling! |
|
@nobrakal Thanks again, merged! |
This is related to #95
So:
FoldableandTraversableinstance from concerned Graph data-types.Foldableinstances.isEmpty,hasVertex,vertexCount,vertexList,vertexSet,vertexIntSetandboxfromAlgebra.Graph.HigherKinded.Classbecause of the removal of theFoldableconstraint.meshandtoruswere rewrited withoutbox.Algebra.Graph.Labelledbut I had to add afoldgUnDiffand a realhasVertex(becauseelemwas used previously).Foldableinstance inAlgebra.Graph.ToGraph(I don't know if it is in the right module).