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

add equational_result attribute for tagging implications and negated implications #93

Merged
merged 13 commits into from
Sep 30, 2024

Conversation

dwrensha
Copy link
Contributor

@dwrensha dwrensha commented Sep 29, 2024

Adds an attribute @[equational_result] to be used for tagging theorems that represent proven implications or nonimplications in the graph.

Collects these results in an environment extension. The idea is that anyone who wants to analyze the data about what's been proven can grab the data from that extension. The data includes the filename where the result lives, and can include any other metadata we want to include.

Previously we had discussed getting the same information by scanning through the environment and collecting all theorems that have the correct shape. That approach would have the advantage of allowing the theorems to be more succinct in the source code (no need to add an attribute). However, I prefer the approach taken by this PR, because:

  • If a theorem does not quite match the correct shape, the attribute handler can report the error immediately, at the theorem declaration.
  • Recording the results (via an environment extension) just once avoids the need to linearly scan the whole environment (could get expensive) at each downstream usage point.

@Command-Master
Copy link
Contributor

Shouldn't ParseImplications now be edited to use this instead of iterating over all theorems?

@Command-Master
Copy link
Contributor

Also, there is duplication of Entry and EntryVariant in Conjecture and Result which should be extracted to a common definition

@dwrensha
Copy link
Contributor Author

Shouldn't ParseImplications now be edited to use this instead of iterating over all theorems?

Yes, it should. There are some details to be worked out about how exactly to organize the shared logic.

@teorth
Copy link
Owner

teorth commented Sep 29, 2024

Just a reminder to modify CONTRIBUTING.md to change the recommended syntax if this proposal gets implemented (it sounds reasonable to me so long as it doesn’t somehow break other Lean functionality concerning theorems)

@Command-Master
Copy link
Contributor

Using result instead of theorem should also be updated in other files

@pitmonticone pitmonticone added do-not-merge This PR is not meant to be merged and removed do-not-merge This PR is not meant to be merged labels Sep 29, 2024
Comment on lines 8 to 12
unsafe def main (args : List String) : IO Unit := do
let module := ← match args with
| [] => pure `equational_theories
| [mod] => pure mod.toName
| _ => throw <| IO.userError "usage: extract_impliations MODULE"
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you can use the CLI from #102

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could make sense in a later PR. For now, I want to focus on getting the core functionality here landed.

@dwrensha dwrensha changed the title add result command. like conjecture, but for things with proofs add equational_result attribute for tagging implications and negated implications Sep 29, 2024
@dwrensha
Copy link
Contributor Author

Shouldn't ParseImplications now be edited to use this instead of iterating over all theorems?

done.

@dwrensha
Copy link
Contributor Author

Also, there is duplication of Entry and EntryVariant in Conjecture and Result which should be extracted to a common definition

The types do have differences and I don't see a benefit to trying to unify them.

It's possible that we may later decide to combine the conjecture and equational_result environment extensions. But for now I think this makes sense as-is.

@dwrensha
Copy link
Contributor Author

Using result instead of theorem should also be updated in other files

In order to avoid making this PR unwieldy, I think it's best to defer updating the other files to later PRs.

@dwrensha dwrensha marked this pull request as ready for review September 29, 2024 15:13
@Command-Master
Copy link
Contributor

Also, there is duplication of Entry and EntryVariant in Conjecture and Result which should be extracted to a common definition

The types do have differences and I don't see a benefit to trying to unify them.

It's possible that we may later decide to combine the conjecture and equational_result environment extensions. But for now I think this makes sense as-is.

What are the differences? To me they seem like the same thing, except one is missing a proof. The benefit of merging them is that later code, e.g. transitive closure, could use the same interface to process both of them

Comment on lines +14 to +16
| nonimplication : Implication → EntryVariant
/-- An equation that always holds. -/
| unconditional : String → EntryVariant
Copy link
Collaborator

Choose a reason for hiding this comment

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

These two are just instance of the general “∃ magma G such that equations {1,2,3} hold and {4,5,6} do not hold”. Note that that already entails 9 non-implicaitons. Once we add more exhaustive analysis of example magmas (as in #19), listing all non-implications here will not scale, and we’ll probably have to switch to the more compact representation.

Doesn't have to happen in this PR, of course; this is just a heads up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that we will want to add other ways to represent results. That can happen later as needed.

I don't see how "EquationN holds for all magmas" is an instance of "∃ magma G such that equations {1,2,3} hold and {4,5,6} do not hold".

Copy link
Collaborator

Choose a reason for hiding this comment

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

A, I misread the comment. My bad

equational_theories/EquationalResult.lean Outdated Show resolved Hide resolved
equational_theories/EquationalResult.lean Outdated Show resolved Hide resolved
@dwrensha
Copy link
Contributor Author

Also, there is duplication of Entry and EntryVariant in Conjecture and Result which should be extracted to a common definition

The types do have differences and I don't see a benefit to trying to unify them.
It's possible that we may later decide to combine the conjecture and equational_result environment extensions. But for now I think this makes sense as-is.

What are the differences? To me they seem like the same thing, except one is missing a proof. The benefit of merging them is that later code, e.g. transitive closure, could use the same interface to process both of them

conjecture currently has an other variant because I wanted to support the ability to make conjectures whose types don't fit the enumerated shapes.

I could imagine that if we add new variants to support e.g. compactly representing many negated implications at once, that such things might only make sense as fully baked results, not as conjectures.

I could see it making sense to merge these into one environment extension or to keep them separate. Keeping them separate seems like the less invasive thing right now. We can adapt in the future. I think more important than nailing down this backend interface is putting the frontend in place, i.e. the conjecture command and the equational_result attribute, so that everyone else working on the project can start using them.

@teorth teorth merged commit 26efcb9 into teorth:main Sep 30, 2024
2 checks passed
@dwrensha dwrensha deleted the collect-results branch September 30, 2024 11:37
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 this pull request may close these issues.

5 participants