Skip to content
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

Use bulk IntMap operations #40

Closed
wants to merge 1 commit into from
Closed

Conversation

treeowl
Copy link
Contributor

@treeowl treeowl commented Aug 30, 2016

Use Data.IntMap.differenceWith to implement (&) and match
for Data.Graph.Inductive.PatriciaTree. Instead of modifying
the graph manually, one key at a time, differenceWith will
efficiently partition the set of keys to be modified along the
structure of the graph. This should be considerably more efficient
when inserting or matching on well-connected nodes.

Fixes #39

Use `Data.IntMap.differenceWith` to implement `(&)` and `match`
for `Data.Graph.Inductive.PatriciaTree`. Instead of modifying
the graph manually, one key at a time, `differenceWith` will
efficiently partition the set of keys to be modified along the
structure of the graph. This should be considerably more efficient
when inserting or matching on well-connected nodes.

Fixes haskell#39
@treeowl
Copy link
Contributor Author

treeowl commented Aug 30, 2016

We may want to change this a bit, actually. I started to make the change, then got confused and stopped. The differenceWith approach is not great when the node is not well-connected. We can actually detect this by building the connection IntMaps manually with foldl', counting as we go, rather than using the (entirely equivalent) fromListWith.

@treeowl
Copy link
Contributor Author

treeowl commented Aug 30, 2016

Hmm.... Actually, I'm not sure if it's worth trying to specialize for the poorly-connected case. Do you have any benchmarks that this PR could be run against?

@treeowl
Copy link
Contributor Author

treeowl commented Aug 30, 2016

Unfortunately, it seems we do have to worry about the poorly-connected case; the benchmarks show a regression. I'm working on it. It's not so clear to me just what the benchmarked graphs tend to look like; that might be nice to document.

@treeowl
Copy link
Contributor Author

treeowl commented Aug 30, 2016

Let me take that back again! The benchmark change must have been some insignificant artifact, because the benchmarks don't actually touch any of that code. We definitely need more benchmarks!

@treeowl treeowl closed this Aug 30, 2016
@ivan-m
Copy link
Contributor

ivan-m commented Aug 30, 2016

Yes, benchmarks are solely lacking.

Alas, I have little excuses to do graph Haskell usage at work (as the few times I thought it may have been relevant for some data processing it ended up running for so long that I killed it as it looked untenable) so I don't have any ready made large-scale graph examples to use :/

@treeowl
Copy link
Contributor Author

treeowl commented Aug 30, 2016

Another little issue is that the benchmark framework is ancient and weird.
microbench has made exactly one release, in 2008. Looking at the
documentation, I just realized I've been using it wrong. But that's partly
explained by the fact that it's a bit hard to see how one can use it right
for graph building. We should almost certainly be using Criterion instead.

On Tue, Aug 30, 2016 at 7:35 PM, Ivan Lazar Miljenovic <
[email protected]> wrote:

Yes, benchmarks are solely lacking.

Alas, I have little excuses to do graph Haskell usage at work (as the few
times I thought it may have been relevant for some data processing it ended
up running for so long that I killed it as it looked untenable) so I don't
have any ready made large-scale graph examples to use :/


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#40 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABzi_ct7FQM6IOXZj51abHWkLwH-wcF8ks5qlL49gaJpZM4JwKVB
.

@ivan-m
Copy link
Contributor

ivan-m commented Aug 30, 2016

Definitely; I just hadn't gotten around to doing new benchmarks yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants