Skip to content

chore: more squeezed simps#11330

Closed
adomani wants to merge 3 commits intomasterfrom
adomani/squeeze_simps2
Closed

chore: more squeezed simps#11330
adomani wants to merge 3 commits intomasterfrom
adomani/squeeze_simps2

Conversation

@adomani
Copy link
Copy Markdown
Contributor

@adomani adomani commented Mar 12, 2024

One more PR with simps squeezed thanks to #11246.


Open in Gitpod

@ghost ghost added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Mar 13, 2024
@ghost ghost removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Mar 13, 2024

theorem H₄IsCoxeter : IsCoxeter H₄ where
symmetric := by simp [Matrix.IsSymm]; decide
symmetric := by unfold Matrix.IsSymm; decide
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess these could have been simp with decide turned on instead? I'm a bit wary of these. Decide is a good example of a tactic that doesn't card too much what the goal is before but might run a lot slower if the input is in a different form

Copy link
Copy Markdown
Contributor

@YaelDillies YaelDillies left a comment

Choose a reason for hiding this comment

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

!bench


theorem G₂IsCoxeter : IsCoxeter G₂ where
symmetric := by simp [Matrix.IsSymm]; decide
symmetric := by unfold Matrix.IsSymm; decide
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you add an instance for DecidablePred Matrix.IsSymm (which should be pretty trivial), you will be able to golf this even further to

Suggested change
symmetric := by unfold Matrix.IsSymm; decide
symmetric := by decide

@ghost ghost added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Mar 23, 2024
Copy link
Copy Markdown
Contributor Author

@adomani adomani left a comment

Choose a reason for hiding this comment

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

Thanks for the reviews! I am skeptical about adding the Decidable instance for Matrix.IsSymm in the same PR.

Also, these issues are still using the old linter. I have not yet had the time to really think through the new one, though.

@adomani
Copy link
Copy Markdown
Contributor Author

adomani commented Mar 24, 2024

!bench

@leanprover-bot
Copy link
Copy Markdown
Collaborator

Here are the benchmark results for commit 6cde597.
There were no significant changes against commit ee18bf3.

@YaelDillies YaelDillies added awaiting-author A reviewer has asked the author a question or requested changes. and removed awaiting-review labels Mar 25, 2024
@joneugster
Copy link
Copy Markdown
Contributor

Same as in #11281: I understood that the implementation of the "flexible linter" addresses this PR and therefore this can be closed.

Please reopen if that's not correct!

@joneugster joneugster closed this Sep 11, 2024
@YaelDillies YaelDillies deleted the adomani/squeeze_simps2 branch August 15, 2025 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-author A reviewer has asked the author a question or requested changes. merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants