{"action":"created","issue":{"url":"https://api.github.com/repos/snowleopard/alga/issues/151","repository_url":"https://api.github.com/repos/snowleopard/alga","labels_url":"https://api.github.com/repos/snowleopard/alga/issues/151/labels{/name}","comments_url":"https://api.github.com/repos/snowleopard/alga/issues/151/comments","events_url":"https://api.github.com/repos/snowleopard/alga/issues/151/events","html_url":"https://github.com/snowleopard/alga/issues/151","id":386271975,"node_id":"MDU6SXNzdWUzODYyNzE5NzU=","number":151,"title":"`ToGraph` maybe doesn't need so many methods?","user":{"login":"michaelpj","id":1699466,"node_id":"MDQ6VXNlcjE2OTk0NjY=","avatar_url":"https://avatars2.githubusercontent.com/u/1699466?v=4","gravatar_id":"","url":"https://api.github.com/users/michaelpj","html_url":"https://github.com/michaelpj","followers_url":"https://api.github.com/users/michaelpj/followers","following_url":"https://api.github.com/users/michaelpj/following{/other_user}","gists_url":"https://api.github.com/users/michaelpj/gists{/gist_id}","starred_url":"https://api.github.com/users/michaelpj/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/michaelpj/subscriptions","organizations_url":"https://api.github.com/users/michaelpj/orgs","repos_url":"https://api.github.com/users/michaelpj/repos","events_url":"https://api.github.com/users/michaelpj/events{/privacy}","received_events_url":"https://api.github.com/users/michaelpj/received_events","type":"User","site_admin":false},"labels":[{"id":546637788,"node_id":"MDU6TGFiZWw1NDY2Mzc3ODg=","url":"https://api.github.com/repos/snowleopard/alga/labels/design","name":"design","color":"5319e7","default":false},{"id":494423163,"node_id":"MDU6TGFiZWw0OTQ0MjMxNjM=","url":"https://api.github.com/repos/snowleopard/alga/labels/enhancement","name":"enhancement","color":"84b6eb","default":true}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2018-11-30T17:02:53Z","updated_at":"2018-12-02T21:42:21Z","closed_at":null,"author_association":"NONE","body":"I understand that the intention of `ToGraph`, like `Foldable`, is that some of the methods can be overridden to provide better performance.\r\n\r\nBut in practice it seems like the only ones that actually matter are \r\n- `foldG` (special implementation for `Fold`)\r\n- `toGraph`, `toAdjacencyMap`, `toAdjacencyIntMap` (special implementations for those very types or similar ones)\r\n\r\nAFAICT the overriding implementations are all equivalent to calling the default implementations on `ToGraph` with the specialised version of e.g. `toAdjacencyMap`. For example, `LabelledAdjacencyMap` just delegates to the `AdjacencyMap` functions after calling `skeleton`... which is just its definition of `toAdjacencyMap`.\r\n\r\nSo I think we could have a `ToGraph` class that just had `foldG` and the `toX` methods, and then provide some generic implementations of the others as functions with a `ToGraph` constraint, rather than class methods.\r\n\r\nIs this actually a problem? Not enormously, it just looks odd to a reader to have such a profusion of class methods, which will potentially grow indefinitely as more algorithms are added.\r\n\r\n(I might just be missing some overrides that are in fact performance-critical, of course!)"},"comment":{"url":"https://api.github.com/repos/snowleopard/alga/issues/comments/443544852","html_url":"https://github.com/snowleopard/alga/issues/151#issuecomment-443544852","issue_url":"https://api.github.com/repos/snowleopard/alga/issues/151","id":443544852,"node_id":"MDEyOklzc3VlQ29tbWVudDQ0MzU0NDg1Mg==","user":{"login":"michaelpj","id":1699466,"node_id":"MDQ6VXNlcjE2OTk0NjY=","avatar_url":"https://avatars2.githubusercontent.com/u/1699466?v=4","gravatar_id":"","url":"https://api.github.com/users/michaelpj","html_url":"https://github.com/michaelpj","followers_url":"https://api.github.com/users/michaelpj/followers","following_url":"https://api.github.com/users/michaelpj/following{/other_user}","gists_url":"https://api.github.com/users/michaelpj/gists{/gist_id}","starred_url":"https://api.github.com/users/michaelpj/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/michaelpj/subscriptions","organizations_url":"https://api.github.com/users/michaelpj/orgs","repos_url":"https://api.github.com/users/michaelpj/repos","events_url":"https://api.github.com/users/michaelpj/events{/privacy}","received_events_url":"https://api.github.com/users/michaelpj/received_events","type":"User","site_admin":false},"created_at":"2018-12-02T21:42:21Z","updated_at":"2018-12-02T21:42:21Z","author_association":"NONE","body":"Hm, it seems like many of these do in fact have at least the potential for more efficient implementations.\r\n\r\nI would be tempted to split out the \"can be folded into a graph\" class from the \"overridable graph operations class\" for clarity, but that's somewhat subjective and based on my confusion on first discovering the class.\r\n\r\ni.e. something like\r\n```\r\nclass ToGraph t where\r\n  toGraph :: ...\r\n  foldg :: ...\r\n\r\nclass ToGraph t => GraphOps t where\r\n   hasVertex :: ...\r\n   ...\r\n```\r\nYou could even drop the `ToGraph` constraint and just offer default signatures that add it.\r\n\r\n> This is not accidental. I just don't know how to make it a method of the ToGraph type class, because it returns a doubly-layered graph. The best we could do is something like this:\r\n\r\nCouldn't you concretely return an `AdjacencyMap (NonEmpty.AdjacencyMap a)` in this case? It's not maximally generic, but I'm not sure how much that matters."}}
{"actor":{"display_login":"michaelpj"}}