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

[Merged by Bors] - chore: golf using filter_upwards #11208

Closed
wants to merge 14 commits into from

Conversation

grunweg
Copy link
Collaborator

@grunweg grunweg commented Mar 6, 2024

This is presumably not exhaustive, but covers about a hundred instances.

Style opinions (e.g., why a particular change is great/not a good idea) are very welcome; I'm still forming my own.


Open in Gitpod

@grunweg grunweg added awaiting-review The author would like community review of the PR awaiting-CI labels Mar 6, 2024
Copy link
Collaborator

@loefflerd loefflerd left a comment

Choose a reason for hiding this comment

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

This PR is definitely an improvement but I saw a couple of points where it can be improved further using the filter_upwards [foo] with bar syntax. There might be more, so have a look for cases where Eventually.mp (or Eventually.and etc) is used just before a filter_upwards.

Mathlib/Probability/Kernel/IntegralCompProd.lean Outdated Show resolved Hide resolved
Mathlib/MeasureTheory/Constructions/Prod/Integral.lean Outdated Show resolved Hide resolved
Mathlib/Analysis/Calculus/Rademacher.lean Show resolved Hide resolved
@loefflerd loefflerd added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Mar 16, 2024
@grunweg
Copy link
Collaborator Author

grunweg commented Mar 16, 2024

Thanks for the hint: I pushed a few more golfs, including looking for occurrences of Eventually.{mp,and}.
awaiting-review

@github-actions github-actions bot added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Mar 16, 2024
Copy link
Collaborator

@loefflerd loefflerd left a comment

Choose a reason for hiding this comment

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

I've checked through all of these now. Most of them look great. I have, however, flagged up a few instances where I think you've been a bit trigger-happy. In particular, if eventually_of_forall or ae_of_all is one ingredient in a long compound proof term (as argument to refine or exact), using the filter_upwards tactic requires breaking this into two tactic calls, and in many cases the result is longer than the original (as well as being, I suspect, fractionally slower to compile).

Mathlib/Analysis/Convolution.lean Outdated Show resolved Hide resolved
Mathlib/Analysis/Fourier/AddCircle.lean Outdated Show resolved Hide resolved
Mathlib/Analysis/Fourier/FourierTransform.lean Outdated Show resolved Hide resolved
Mathlib/Analysis/Fourier/FourierTransformDeriv.lean Outdated Show resolved Hide resolved
Mathlib/Analysis/MellinTransform.lean Outdated Show resolved Hide resolved
Mathlib/MeasureTheory/Integral/DominatedConvergence.lean Outdated Show resolved Hide resolved
Mathlib/MeasureTheory/Integral/IntervalIntegral.lean Outdated Show resolved Hide resolved
Mathlib/NumberTheory/ZetaFunction.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Kernel/IntegralCompProd.lean Outdated Show resolved Hide resolved
@loefflerd loefflerd added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Mar 19, 2024
@grunweg
Copy link
Collaborator Author

grunweg commented Mar 19, 2024

Thank you, all fixed. (Sorry for going overboard - I rather wanted you to tell me "this is not better" than the other way. Now I know.)
awaiting-review

@github-actions github-actions bot added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Mar 19, 2024
Copy link
Collaborator

@loefflerd loefflerd left a comment

Choose a reason for hiding this comment

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

This looks good now. There are surely further golfs that are possible; but this PR certainly is a substantial improvement on the existing code, and given that it touches rather a lot of files, it will rot quickly if we try to make it perfect.

maintainer merge

Copy link

🚀 Pull request has been placed on the maintainer queue by loefflerd.

@fpvandoorn
Copy link
Member

This looks good, thanks!

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review The author would like community review of the PR labels Mar 20, 2024
mathlib-bors bot pushed a commit that referenced this pull request Mar 20, 2024
This is presumably not exhaustive, but covers about a hundred instances.

Style opinions (e.g., why a particular change is great/not a good idea) are very welcome; I'm still forming my own.
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Mar 20, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title chore: golf using filter_upwards [Merged by Bors] - chore: golf using filter_upwards Mar 20, 2024
@mathlib-bors mathlib-bors bot closed this Mar 20, 2024
@mathlib-bors mathlib-bors bot deleted the MR-filter-upwards branch March 20, 2024 12:03
mathlib-bors bot pushed a commit that referenced this pull request Mar 20, 2024
Sometimes, that line can be golfed into the next line.
Inspired by a [comment](#11208 (comment)) of @loefflerd; any decisions are my own.
dagurtomas pushed a commit that referenced this pull request Mar 22, 2024
This is presumably not exhaustive, but covers about a hundred instances.

Style opinions (e.g., why a particular change is great/not a good idea) are very welcome; I'm still forming my own.
dagurtomas pushed a commit that referenced this pull request Mar 22, 2024
Sometimes, that line can be golfed into the next line.
Inspired by a [comment](#11208 (comment)) of @loefflerd; any decisions are my own.
utensil pushed a commit that referenced this pull request Mar 26, 2024
This is presumably not exhaustive, but covers about a hundred instances.

Style opinions (e.g., why a particular change is great/not a good idea) are very welcome; I'm still forming my own.
utensil pushed a commit that referenced this pull request Mar 26, 2024
Sometimes, that line can be golfed into the next line.
Inspired by a [comment](#11208 (comment)) of @loefflerd; any decisions are my own.
Louddy pushed a commit that referenced this pull request Apr 15, 2024
This is presumably not exhaustive, but covers about a hundred instances.

Style opinions (e.g., why a particular change is great/not a good idea) are very welcome; I'm still forming my own.
Louddy pushed a commit that referenced this pull request Apr 15, 2024
Sometimes, that line can be golfed into the next line.
Inspired by a [comment](#11208 (comment)) of @loefflerd; any decisions are my own.
atarnoam pushed a commit that referenced this pull request Apr 16, 2024
This is presumably not exhaustive, but covers about a hundred instances.

Style opinions (e.g., why a particular change is great/not a good idea) are very welcome; I'm still forming my own.
atarnoam pushed a commit that referenced this pull request Apr 16, 2024
Sometimes, that line can be golfed into the next line.
Inspired by a [comment](#11208 (comment)) of @loefflerd; any decisions are my own.
uniwuni pushed a commit that referenced this pull request Apr 19, 2024
This is presumably not exhaustive, but covers about a hundred instances.

Style opinions (e.g., why a particular change is great/not a good idea) are very welcome; I'm still forming my own.
uniwuni pushed a commit that referenced this pull request Apr 19, 2024
Sometimes, that line can be golfed into the next line.
Inspired by a [comment](#11208 (comment)) of @loefflerd; any decisions are my own.
callesonne pushed a commit that referenced this pull request Apr 22, 2024
This is presumably not exhaustive, but covers about a hundred instances.

Style opinions (e.g., why a particular change is great/not a good idea) are very welcome; I'm still forming my own.
callesonne pushed a commit that referenced this pull request Apr 22, 2024
Sometimes, that line can be golfed into the next line.
Inspired by a [comment](#11208 (comment)) of @loefflerd; any decisions are my own.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer-merge ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants