-
Notifications
You must be signed in to change notification settings - Fork 534
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
feat: import LeanSAT LRAT #5074
Conversation
Mathlib CI status (docs):
|
!bench |
(I'm not going to complain whatever |
Here are the benchmark results for commit 66fa475. Benchmark Metric Change
===================================================================
- import Lean branches 1.1% (21.4 σ)
- import Lean instructions 1.1% (24.1 σ)
- import Lean maxrss 1.5% (59.4 σ)
- reduceMatch maxrss 1.4% (189.0 σ)
- stdlib fix level params 4.4% (19.8 σ)
- stdlib instantiate metavars 47.4% (101.0 σ)
- stdlib instructions 2.7% (4089.0 σ)
- stdlib maxrss 1.5% (1022.2 σ)
- stdlib share common exprs 3.8% (46.1 σ)
- stdlib tactic execution 16.2% (30.2 σ)
- stdlib task-clock 2.5% (15.2 σ)
- stdlib size bytes .olean 1.8%
- stdlib size lines 2.7%
- tests/bench/ interpreted maxrss 1.4% (964.6 σ)
- workspaceSymbols maxrss 1.4% (106.9 σ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably makes sense to make one quick pass over the naming; I left some examples. Apart from that I guess there's not much point in spending too much time on the internal stuff as long as the performance of the LRAT.check
function is good.
This PR imports LeanSAT's LRAT module as step 4/~6 (step 7 could go after I did some refactorings to import this) of the LeanSAT upstreaming. It is the last large component, after this only the LRAT parser and the reflection tactic that hooks everything up to the meta level remains. In particular it is the last component that contains notable proofs, yay!
Again a few remarks:
Std
? I'm not quite sure whether it should be there. At the current level of code/proof quality we can certainly not import the checker itself intoStd
but maybe having the data type as well as the trimming algorithm there might be of interested? I'm hoping that as we refactor the checker in the future its quality will be high enough to be also put intoStd
. At this point we would have a full AIG -> CNF -> LRAT verification pipeline inStd
for everyone to use. One additional blocker in this is that we cannot provide the parsers for the format inStd
as of today becauseParsec
is still inLean
so that would also have to change.LRAT
directory. It basically only contains the LRAT format itself, the checker + soundness proof and the trimming algorithm. As long as we don't need to change their API (which we shouldn't have to I think) we can always swap out the entireInternal
directory without breaking anything else in LeanSAT.Internal
module itself contains another layer of abstraction in the form of theFormula
class. This allows us to swap out the most complex component inInternal
as well, without having to touch any of the infrastructure that is built around it either.Internal
module. In my experience over upgrading to many nightlies during the course of LeanSAT development, I have gotten these proofs cleaned up to the point, where they only break if we change theList
orArray
proof API significantly. Given that we are currently in the process of stabilizing it I'm hoping that these proofs do not have to be touched anymore unless we do something crazy. All of the custom theory that the LRAT component developed around various basic data types has been upstreamed into Lean over the course of various other PRs.Internal
and in particularInternal.Formula
(this module is not for the light-hearted Lean reviewer) I'm all for it. Otherwise the best course of action to provide LeanSAT to our users soon would probably be to merge it as is and do a cut + rewrite at one of the two interface points described above.