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

simp should try more specific lemmas first #4173

Closed
nomeata opened this issue May 15, 2024 · 3 comments · Fixed by #4434
Closed

simp should try more specific lemmas first #4173

nomeata opened this issue May 15, 2024 · 3 comments · Fixed by #4434

Comments

@nomeata
Copy link
Collaborator

nomeata commented May 15, 2024

Consider the following code

set_option autoImplicit false 

inductive DOW where
 | monday | tuesday | wednesday | thursday | friday | saturday | sunday

 def DOW.isWorkday (n : Nat) (d : DOW) : Bool :=
  match d with
   | monday | tuesday | wednesday | thursday | friday => true
   | _ => match n with | 0 => false | n+1 => d.isWorkday n

example : DOW.friday.isWorkday 0 := by
  set_option trace.Meta.Tactic.simp true in
  simp  [DOW.isWorkday]

/-- info: DOW.isWorkday.eq_5 (n : Nat) : DOW.isWorkday n DOW.friday = true -/
#guard_msgs in
#check DOW.isWorkday.eq_5
/--
info: DOW.isWorkday.eq_6 (d : DOW) (x_1 : d = DOW.monday → False) (x_2 : d = DOW.tuesday → False)
  (x_3 : d = DOW.wednesday → False) (x_4 : d = DOW.thursday → False) (x_5 : d = DOW.friday → False) :
  DOW.isWorkday 0 d = false
-/
#guard_msgs in
#check DOW.isWorkday.eq_6

The recursion in DOW.isWorkday is there so that we get equational theorems; with #3983 this would apply to a non-recursive function as well.

The lemma DOW.isWorkday.eq_5 should solve this rather quickly. But as the trace reveals, simp tries the rather more complicated DOW.isWorkday.eq_6 first!

(In fact, it does something even weirder that I don’t fully understand, unfolding the definition and then trying to appliy .match_2.eq_n lemmas, or so; the trace is far longer than I would have expected here, so there may be another issue lurking here.)

A possible fix, explored in #4158, might be to tweak the DiscrTree to return more specific lemmas first, but there might be other approaches (e.g. ordering all lemmas returned from the discrtree somehow, maybe trying unconstrainted lemmas before those with side-conditions), hence opening this as an issue.

@JovanGerb
Copy link
Contributor

JovanGerb commented May 26, 2024

In type class synthesis, this code gets the result from the discrimination tree:

      let result ← globalInstances.getUnify type tcDtConfig
      -- Using insertion sort because it is stable and the array `result` should be mostly sorted.
      -- Most instances have default priority.
      let result := result.insertionSort fun e₁ e₂ => e₁.priority < e₂.priority

They purposefully use a stable sorting algorithm, and the results are used in the order back-to-front in the array. My interpretation is that this is good because the patterns with more * patterns are at the start of the array. The implementation of DiscrTree is that whenever it has to choose between a star and a non-star search branch, it does the star branch first.

This means that earlier results aren't necessarily always more general than later results, but it does means that earlier results are usually more general than later results

@JovanGerb
Copy link
Contributor

In my RefinedDiscrTree, I output a natural number score that indicates how good the match is, and I sort the result based on the scores of the entries. This a better way of determining which lemmas are more/less specific. This is especially useful for a human-interactive library search, because you really want to show the more relevant results first. It would be possible to also add this to Lean's DiscrTree, or we could see if simp works with RefinedDiscrTree.

@nomeata
Copy link
Collaborator Author

nomeata commented May 26, 2024

Thanks for the cross-references, that's helpful! It also means that my experiment in #4158 may have affected type class inference negatively, which may skew perf results.

nomeata added a commit that referenced this issue Jun 12, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 17, 2024
…#4434)

This assigns priorities to the equational lemmas so that more specific
ones
are tried first before a possible catch-all with possible
side-conditions.

We assign very low priorities to match the simplifiers behavior when
unfolding
a definition, which happens in `simpLoop`’ `visitPreContinue` after
applying
rewrite rules.

Definitions with more than 100 equational theorems will use priority 1
for all
but the last (a heuristic, not perfect).

fixes #4173, to some extent.
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 a pull request may close this issue.

2 participants