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

fix: abstractNestedProofs should see into the head of an application #7353

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JovanGerb
Copy link
Contributor

@JovanGerb JovanGerb commented Mar 5, 2025

This PR changes abstractNestedProofs so that it also visits the subterms in the head of an application.

This oversight caused some definitions in mathlib to have unabstracted proofs, such as CategoryTheory.StructuredArrow.commaMapEquivalenceInverse

Mathlib bench:
build instructions -0,141 %
process pre-definitions 5,541 %

There is a small general slowdown from the extra work done by abstractNestedProofs . But this is outweighed by the speedup that we get in files related to CategoryTheory.Functor.

Zulip: https://leanprover.zulipchat.com/#narrow/channel/270676-lean4/topic/dsimp.20simplifies.20proofs.2C.20which.20is.20slow/near/503630173

@JovanGerb JovanGerb marked this pull request as draft March 5, 2025 22:31
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Mar 5, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Mar 5, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Mar 5, 2025
@leanprover-community-bot leanprover-community-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Mar 5, 2025
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented Mar 5, 2025

Mathlib CI status (docs):

  • ✅ Mathlib branch lean-pr-testing-7353 has successfully built against this PR. (2025-03-05 23:49:54) View Log
  • ✅ Mathlib branch lean-pr-testing-7353 has successfully built against this PR. (2025-03-06 00:53:45) View Log
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase ca0d8226192e7c0cdcc71d6322c3226ad4f73f30 --onto 5536281238dff2fb4e0a54da472d4f0d6496069e. You can force Mathlib CI using the force-mathlib-ci label. (2025-03-06 08:40:09)

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Mar 6, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Mar 6, 2025
@JovanGerb JovanGerb marked this pull request as ready for review March 6, 2025 03:30
Copy link
Collaborator

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Especially as I'm also experimenting with abstracting more proofs at the moment.

@nomeata
Copy link
Collaborator

nomeata commented Mar 6, 2025

Can you fix the tests (again)?

@JovanGerb
Copy link
Contributor Author

JovanGerb commented Mar 6, 2025

Hmm, this test is really annoying because it keeps changing the order of its messages (hence the merge conflict)

@nomeata nomeata added the changelog-language Language features, tactics, and metaprograms label Mar 6, 2025
@nomeata nomeata enabled auto-merge March 6, 2025 08:01
auto-merge was automatically disabled March 6, 2025 08:09

Head branch was pushed to by a user without write access

@nomeata nomeata enabled auto-merge March 6, 2025 08:13
@nomeata nomeata disabled auto-merge March 6, 2025 08:22
@nomeata
Copy link
Collaborator

nomeata commented Mar 6, 2025

Actually, given that this is a performance critical function, could you maybe make another measurement with the old code that looks at a full application as a whole (but of course fixed to also visit the function). Partial applications are never proofs, and it may make a difference to not look up the cache for every partial application in the term. (Or maybe the proposed code is better if partial applications are shared - who knows)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builds-mathlib CI has verified that Mathlib builds against this PR changelog-language Language features, tactics, and metaprograms toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants