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

Equations command modification #427

Merged
merged 14 commits into from
Oct 9, 2024

Conversation

0art0
Copy link
Contributor

@0art0 0art0 commented Oct 8, 2024

This PR modifies the equation command to automatically generate the corresponding law with variables over Fin n (in addition to the law with variables over Nat that is already being generated).

This is used to show that the automatically generated laws are equivalent to the original equation in terms of satisfiability.

Additionally, this PR introduces an environment extension to store all the equational laws, which can be used to export laws as described in #142.

This PR also modifies a result transporting laws from Fin n to Nat to omit the n != 0 hypothesis.

@goens
Copy link
Contributor

goens commented Oct 8, 2024

this looks good to me!

@pitmonticone
Copy link
Collaborator

Thank you very much @0art0!
Please make sure the project builds correctly by locally running scripts/run_before_push.sh.

@0art0
Copy link
Contributor Author

0art0 commented Oct 8, 2024

Thanks. I seem to be getting a strange build error in one of the generated files:

✖ [3337/3343] Building equational_theories.Generated.EquationSearch.theorems.Run5
trace: .> LEAN_PATH=././.lake/packages/batteries/.lake/build/lib:././.lake/packages/Qq/.lake/build/lib:././.lake/packages/aesop/.lake/build/lib:././.lake/packages/proofwidgets/.lake/build/lib:././.lake/packages/Cli/.lake/build/lib:././.lake/packages/importGraph/.lake/build/lib:././.lake/packages/LeanSearchClient/.lake/build/lib:././.lake/packages/mathlib/.lake/build/lib:././.lake/packages/checkdecls/.lake/build/lib:././.lake/packages/MD4Lean/.lake/build/lib:././.lake/packages/UnicodeBasic/.lake/build/lib:././.lake/packages/BibtexQuery/.lake/build/lib:././.lake/packages/doc-gen4/.lake/build/lib:././.lake/build/lib LD_LIBRARY_PATH= /home/art/.elan/toolchains/leanprover--lean4---v4.13.0-rc3/bin/lean -Dpp.unicode.fun=true -DautoImplicit=false -DrelaxedAutoImplicit=false ././././equational_theories/Generated/EquationSearch/theorems/Run5.lean -R ./././. -o ././.lake/build/lib/equational_theories/Generated/EquationSearch/theorems/Run5.olean -i ././.lake/build/lib/equational_theories/Generated/EquationSearch/theorems/Run5.ilean -c ././.lake/build/ir/equational_theories/Generated/EquationSearch/theorems/Run5.c --json
error: Lean exited with code 137
Some required builds logged failures:
- equational_theories.Generated.EquationSearch.theorems.Run5

I can't immediately think of a reason why the changes introduced in my code would be causing this, but I'll continue to track down the issue.

@pitmonticone
Copy link
Collaborator

Ok @0art0. I will take a look at it later.

Can you please resolve the merge conflicts?

@0art0
Copy link
Contributor Author

0art0 commented Oct 8, 2024

@pitmonticone Thank you so much. I've merged conflicts on both PRs.

@pitmonticone pitmonticone self-assigned this Oct 8, 2024
@pitmonticone pitmonticone marked this pull request as draft October 8, 2024 22:07
@Shreyas4991
Copy link
Collaborator

@0art0 : Is this PR ready for reviewing? I believe there is another PR that is dependent on this one as well.

@Shreyas4991 Shreyas4991 marked this pull request as ready for review October 8, 2024 22:54
@0art0
Copy link
Contributor Author

0art0 commented Oct 8, 2024

Yes, it's ready for reviewing. Thank you for helping out with fixing the build issues. The PR that depends on this one is #429.

@0art0
Copy link
Contributor Author

0art0 commented Oct 8, 2024

awaiting-review

@github-actions github-actions bot added the awaiting-review the PR passes CI and is ready for review label Oct 8, 2024
Copy link
Collaborator

@Shreyas4991 Shreyas4991 left a comment

Choose a reason for hiding this comment

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

I wonder if the MagmaNat theorems and the theorems in EquationalLawConversion should have corresponding blueprint entries. @0art0 and @teorth your thoughts? On the one hand these are fairly trivial theorems, on the other hand the blueprints in the past have contained all the tiny lemmata as well.

@0art0
Copy link
Contributor Author

0art0 commented Oct 8, 2024

I think the theorems converting between MagmaLaws over Fin n and Nat can be added to the blueprint. The EquationalLawConversion file is more of an implementation detail, in my opinion; it was a quick hack on my part to bypass the complicated meta-programming that would otherwise be required to convert maps from Fin n to a curried sequence of quantifiers.

@Shreyas4991
Copy link
Collaborator

Shreyas4991 commented Oct 8, 2024

@0art0 : which task does this PR close btw? EDIT : Found it.

About the blueprint: Seems sensible. I'll change this PR to a draft, once the blueprint is ready we can get this merged.

@0art0
Copy link
Contributor Author

0art0 commented Oct 8, 2024

This closes #428. Alright, I can add a note about the conversion between MagmaLaws over Fin n and Nat. I think it would be good for @goens to have a look at it when it's done, since it's the machinery he formalized in #368.

@pitmonticone
Copy link
Collaborator

The action didn't timeout, but it still took 53 minutes to run the project. Is this expected?

I'm inclined to merge both PRs (#427 and #429) because they look fine to me (even if the branch of #429 seem to have been created from the branch of #427 which isn't considered best practice), but I may be missing something.

See discussion here https://leanprover.zulipchat.com/#narrow/stream/458659-Equational/topic/Do.20we.20need.20to.20build.20EquationalSearch.20files.3F/near/475682423

@Shreyas4991
Copy link
Collaborator

Let's wait for the blueprint to be updated

@0art0
Copy link
Contributor Author

0art0 commented Oct 9, 2024

I've added a section to the blueprint.

@pitmonticone
Copy link
Collaborator

Great, the cache mechanism worked properly and now it took just a few minutes.

@pitmonticone
Copy link
Collaborator

Thanks again @0art0!

@pitmonticone pitmonticone merged commit adc92cb into teorth:main Oct 9, 2024
1 check passed
pitmonticone added a commit that referenced this pull request Oct 9, 2024
Exports the equations defined in `Equations.lean` and
`AllEquations.lean` to a JSON file `data/magma_equations.json`.

Depends on PR #427.

---------

Co-authored-by: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com>
@goens
Copy link
Contributor

goens commented Oct 9, 2024

The blueprint looks good to me too! (I presume @teorth would have written down the proof as 'trivial', but I think this level of detail doesn't hurt either)

Comment on lines +77 to +86
-- define law over `Fin n`
elabCommand (← `(command| abbrev%$tk $finLawIdent : Law.MagmaLaw (Fin $freeMagmaSize) := $tl))
-- compatibility between the `finLaw` and the original equation
let modelsIffLemma : Ident := mkIdent (.mkSimple s!"models_iff_{is.size}")
elabCommand (← `(command| abbrev%$tk $finThmName : ∀ (G : Type _) [$inst : Magma G], G ⊧ $finLawIdent ↔ $eqIdent G := $modelsIffLemma $finLawIdent))
-- define the actual law over `Nat`
elabCommand (← `(command| abbrev%$tk $lawIdent : Law.NatMagmaLaw := $tl))
-- compatibility between the law and the original equation
elabCommand (← `(command| abbrev%$tk $thmName : ∀ (G : Type _) [$inst : Magma G], G ⊧ $lawIdent ↔ $eqIdent G :=
fun G _ ↦ Iff.trans (Law.satisfies_fin_satisfies_nat G $finLawIdent).symm ($finThmName G)))
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to do this in a more low-level way, rather than re-elaborating the law repeatedly. A hybrid approach would be to elaborate the law once and use Expr.toSyntax to repeat it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. The law happens to be elaborated slightly differently in the two situations -- in one case, the leaves are coerced to Fin n, while in the second case, they are left as natural numbers, so storing the rest of elaboration as an expression may not directly work. (It's possible that I may be misunderstanding the comment.)

teorth pushed a commit that referenced this pull request Oct 9, 2024
…ld (#465)

#427 has caused builds to become much slower.
If I comment out two lines that it added, as in this PR, build times
become reasonable again.
This is a stopgap until we understand the source of the slowness.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review the PR passes CI and is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants