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

Flexible search #13

Closed
wants to merge 33 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jamii

jamii commented Dec 18, 2012

Replaces the core stream data-structure with a lazy tree of continuations (ITake becomes ISearchTree).

Adds optional fair conjunction (via bind-fair and all-fair).

Adds new search algorithms: bfs-lazy (the original algorithm), bfs-strict, dfs-lazy, dfs-strict, dfs-par (using fork-join).

Unfortunately, introduces performance regressions for lazy search. For example, for bench/zebrao I see:

  • original branch w/ (run* ...): ~30ms
  • original branch w/ (run 1 ...): ~8.5ms
  • this branch w/ (run* ...): ~25ms
  • this branch w/ (run 1 ...): ~27ms

I suspect that this is simply a missing thunk somewhere but I've run out of time to work on this. Given that laziness is now largely controlled by the search algorithm it may also be worth deprecating the various variants of run.

As a consolation prize:

  • this branch w/ (binding [*search* dfs-par] (run* ...)): ~18ms
@swannodette

This comment has been minimized.

Show comment
Hide comment
@swannodette

swannodette Dec 13, 2012

I just realized that the reason many tests are failing are probably simply that your version now returns results in a different order. I need to go through the tests and convert many of them to set comparisons.

swannodette commented on 9be771a Dec 13, 2012

I just realized that the reason many tests are failing are probably simply that your version now returns results in a different order. I need to go through the tests and convert many of them to set comparisons.

@swannodette

This comment has been minimized.

Show comment
Hide comment
@swannodette

swannodette Dec 18, 2012

Member

Excellent, however the usual spiel - I can't merge pull requests for official Clojure projects. I need an enhancement ticket + squashed patch in JIRA, http://dev.clojure.org/jira/browse/LOGIC. I also can only take patches from people that have signed the Contributor Agreement (CA) - http://clojure.org/contributing. Please mail this in, once that's on its way to North Carolina I will happily apply the JIRA patch.

As far as feedback this looks like a fairly small set of changes! To make that even more clear please remove the whitespace changes from the JIRA patch - this is pretty easy to do w/ git if you interactively add the changes via git when creating the patch.

Would love to get this in, this looks like a great step forward for customizable search in core.logic. Thanks much.

Oh and some tests would make me a little bit more confident when applying this :)

Member

swannodette commented Dec 18, 2012

Excellent, however the usual spiel - I can't merge pull requests for official Clojure projects. I need an enhancement ticket + squashed patch in JIRA, http://dev.clojure.org/jira/browse/LOGIC. I also can only take patches from people that have signed the Contributor Agreement (CA) - http://clojure.org/contributing. Please mail this in, once that's on its way to North Carolina I will happily apply the JIRA patch.

As far as feedback this looks like a fairly small set of changes! To make that even more clear please remove the whitespace changes from the JIRA patch - this is pretty easy to do w/ git if you interactively add the changes via git when creating the patch.

Would love to get this in, this looks like a great step forward for customizable search in core.logic. Thanks much.

Oh and some tests would make me a little bit more confident when applying this :)

@jamii

This comment has been minimized.

Show comment
Hide comment
@jamii

jamii Dec 19, 2012

I'll send off the CA on Friday.

In the meantime, I poked around at those performance regressions and I'm now wondering if it's just a matter of different search order. In theory bfs-lazy should return results in the same order as master but some of the tests needed changing, so perhaps I've got something swapped around somewhere...

jamii commented Dec 19, 2012

I'll send off the CA on Friday.

In the meantime, I poked around at those performance regressions and I'm now wondering if it's just a matter of different search order. In theory bfs-lazy should return results in the same order as master but some of the tests needed changing, so perhaps I've got something swapped around somewhere...

@jamii

This comment has been minimized.

Show comment
Hide comment
@jamii

jamii Dec 24, 2012

Sent the CA on Friday.

jamii commented Dec 24, 2012

Sent the CA on Friday.

@swannodette

This comment has been minimized.

Show comment
Hide comment
@swannodette

swannodette Dec 27, 2012

Member

Excellent! Feel free to add a ticket to JIRA w/ attached squashed patch against master.

Member

swannodette commented Dec 27, 2012

Excellent! Feel free to add a ticket to JIRA w/ attached squashed patch against master.

jamii added some commits Dec 27, 2012

Merge branch 'master' into flexible-search
Conflicts:
	src/main/clojure/clojure/core/logic.clj
	src/main/clojure/clojure/core/logic/bench.clj
	src/test/clojure/clojure/core/logic/tests.clj
@jamii

This comment has been minimized.

Show comment
Hide comment
@jamii

This comment has been minimized.

Show comment
Hide comment
@jamii

jamii Jan 11, 2013

I wonder if the poor performance in the parallel solver is related to this - https://groups.google.com/forum/#!msg/clojure/48W2eff3caU/qKjFmh3dgvMJ

jamii commented Jan 11, 2013

I wonder if the poor performance in the parallel solver is related to this - https://groups.google.com/forum/#!msg/clojure/48W2eff3caU/qKjFmh3dgvMJ

@swannodette

This comment has been minimized.

Show comment
Hide comment
@swannodette

swannodette Jan 11, 2013

Member

There's far too much speculation in that thread. Even so, I doubt it.

Member

swannodette commented Jan 11, 2013

There's far too much speculation in that thread. Even so, I doubt it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment