Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

feat(tactic/solve_by_elim): add accept parameter to prune tree search#2245

Merged
mergify[bot] merged 35 commits intomasterfrom
solve_by_elim_accept
Mar 28, 2020
Merged

feat(tactic/solve_by_elim): add accept parameter to prune tree search#2245
mergify[bot] merged 35 commits intomasterfrom
solve_by_elim_accept

Conversation

@kim-em
Copy link
Collaborator

@kim-em kim-em commented Mar 25, 2020

Adds some features to solve_by_elim, to allow pruning of the search tree.

From the doc-string of solve_by_elim.basic_opt:

/--
Configuration options for `solve_by_elim`.

...
* `accept` determines whether the current branch should be explored.
   It takes the current state of original goals as a `list expr` argument,
   and should fail to signal omitting this branch.
   By default `accept := λ _, skip` always succeeds.
* `pre_apply` specifies an additional tactic to run before each round of `apply`.
...
-/

@kim-em kim-em added the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Mar 25, 2020
@kim-em kim-em added awaiting-review The author would like community review of the PR and removed blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. labels Mar 26, 2020
@kim-em kim-em changed the title feat(tactic/solve_by_elim): add accept parameter to prune tree search (blocked by #2222) feat(tactic/solve_by_elim): add accept parameter to prune tree search Mar 26, 2020
@robertylewis
Copy link
Member

It takes the current state of original goals as a list expr argument,

I don't really follow this. (What is a "current state of original goals"?) At least, I have no idea how I'd use this in practice. Could you add an example of when this option is useful and the syntax that would be used to give it?

@kim-em
Copy link
Collaborator Author

kim-em commented Mar 26, 2020

@robertylewis, I've updated the documentation in a few places.

The main doc-string for solve_by_elim now says:

- accept: a subsidiary tactic `list expr → tactic unit` that at each step is passed the original goals
    (which may by now be partially solved by previous `apply` steps).
    If the `accept` tactic fails,
    `solve_by_elim` will abort searching the current branch and backtrack.
    This may be used to filter results, either at every step of the search,
    or filtering complete results
    (by testing for the absence of metavariables, and then the filtering condition).

In the doc-string on the configuration structure that contains accept I've written much the same, and included a pointer to the example in the test file, which I've tidied up and improved the explanation for.

Does this make more sense?

(I use this accept parameter in a fundamental way in the equiv_rw PR I've just made.)

@kim-em
Copy link
Collaborator Author

kim-em commented Mar 26, 2020

I've also add a module doc.

@kim-em kim-em added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Mar 26, 2020
Configuration options for `solve_by_elim`.

* `accept : list expr → tactic unit` determines whether the current branch should be explored.
It is passed the original goals as a `list expr` argument
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this line is still confusing me. I would guess first that it's looking for a list of mvars, and second that it's looking for a list of the types of these mvars. In the code, it looks like you give it neither -- you instantiate the mvars before passing them to accept. Is that optional, or required? What's structure is this list expr argument expected to have?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've tried again.

The idea is that when solve_by_elim is first invoked, we call get_goals, and stash them. Now, at every step in the search tree, we take those "original goals", instantiate metavariables in them (as those original goals have by now been partly solved by previous apply steps), and pass them to the accept tactic. Thus the accept tactic can inspect the history of apply steps that have succeeded up to this point, by looking at how the original metavariables from get_goals have been partially unified.

The main change I made in the docs is to replace phrases like "original goals", explicitly saying that solve_by_elim calls get_goals:

It is passed the original metavariables reported by get_goals when solve_by_elim started, as a list expr argument (these metavariables may by now have been partially solved by previous apply steps), ...

I agree this is non-obvious, and my doc-strings were not adequate, so please tell me if we're still not there yet!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accept is run before the current goals are explored, right? It would be good to clarify that in the docstring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified!

@robertylewis robertylewis added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Mar 27, 2020
Co-Authored-By: Rob Lewis <Rob.y.lewis@gmail.com>
@kim-em kim-em added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Mar 28, 2020
@robertylewis robertylewis added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-review The author would like community review of the PR labels Mar 28, 2020
@mergify mergify bot merged commit ad53e0b into master Mar 28, 2020
@bryangingechen bryangingechen deleted the solve_by_elim_accept branch March 30, 2020 02:24
butterthebuddha pushed a commit to butterthebuddha/mathlib that referenced this pull request May 15, 2020
…leanprover-community#2245)

* chore(tactic/solve_by_elim): cleanup

* cleanup

* what happened to my commit?

* fix

* fix

* fixed?

* Tweak comments

* feat(tactic/solve_by_elim): add accept parameter to prune tree search

* when called with empty lemmas, use the same default set as the interactive tactic

* stop cheating with [] ~ none

* indenting

* various

* various

* docstring

* fix docstrings

* more docs

* docs

* fix doc-strings

* improve documentation of accept, and add doc-string

* improve docs

* try again with documentation

* clarify when accept runs

* Update src/tactic/solve_by_elim.lean

Co-Authored-By: Rob Lewis <Rob.y.lewis@gmail.com>

* Update src/tactic/solve_by_elim.lean

* Update src/tactic/solve_by_elim.lean

Co-authored-by: Rob Lewis <Rob.y.lewis@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
butterthebuddha pushed a commit to butterthebuddha/mathlib that referenced this pull request May 16, 2020
…leanprover-community#2245)

* chore(tactic/solve_by_elim): cleanup

* cleanup

* what happened to my commit?

* fix

* fix

* fixed?

* Tweak comments

* feat(tactic/solve_by_elim): add accept parameter to prune tree search

* when called with empty lemmas, use the same default set as the interactive tactic

* stop cheating with [] ~ none

* indenting

* various

* various

* docstring

* fix docstrings

* more docs

* docs

* fix doc-strings

* improve documentation of accept, and add doc-string

* improve docs

* try again with documentation

* clarify when accept runs

* Update src/tactic/solve_by_elim.lean

Co-Authored-By: Rob Lewis <Rob.y.lewis@gmail.com>

* Update src/tactic/solve_by_elim.lean

* Update src/tactic/solve_by_elim.lean

Co-authored-by: Rob Lewis <Rob.y.lewis@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants