Skip to content

[Merged by Bors] - feat(Tactic.Bound): Add a bound tactic for inequalities by structure#10562

Closed
girving wants to merge 32 commits intomasterfrom
GI_bound
Closed

[Merged by Bors] - feat(Tactic.Bound): Add a bound tactic for inequalities by structure#10562
girving wants to merge 32 commits intomasterfrom
GI_bound

Conversation

@girving
Copy link
Copy Markdown
Collaborator

@girving girving commented Feb 14, 2024

bound proves inequalities that follow "obviously by structure", assuming nonnegativity or positivity of subterms where necessary. It is built on top of Aesop. It has significant overlap with positivity and gcongr, but can also switch back and forth between those modes (such as when proving 0 < a * b - a * c from 0 < a, c < b).

Zulip discussion: https://leanprover.zulipchat.com/#narrow/stream/239415-metaprogramming-.2F-tactics/topic/.60bound.60.20mixes.20.60gcongr.60.20and.20.60positivity.60/near/412120380


Here's an initial implementation of the bound tactic discussed on Zulip, upstreamed from https://github.com/girving/ray/blob/main/Ray/Tactic/Bound.lean. Happy to do whatever cleanup or further testing is deemed useful. Some notes:

  1. mul_inv_le_one_of_nonneg_of_le and Real.pi_nonneg should probably be moved to their appropriate places, but I wanted to check first.
  2. le_self_pow_of_pos is a wart due to bound not knowing what to do with n ≠ 0 goals. One option would be give bound a rule for Nat.pos_iff_ne_zero, so that n ≠ 0 automatically turns into 0 < n.
  3. Bound.lean currently imports Mathlib.Analysis.Analytic.Basic just to add r_pos as a forward rule. If this rule is reasonable it would of course be better to move that to Mathlib.Analysis.Analytic.Basic to remove the heavy import.
  4. I added two more triangle inequalities in a separate PR (which also shows up as a commit within this one until that other PR is dealt with):

Open in Gitpod

@girving girving added awaiting-review t-meta Tactics, attributes or user commands labels Feb 14, 2024
@ghost ghost added blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) and removed blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) labels Feb 14, 2024
@ghost
Copy link
Copy Markdown

ghost commented Feb 15, 2024

@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 Feb 15, 2024
@girving girving removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Feb 15, 2024
@girving
Copy link
Copy Markdown
Collaborator Author

girving commented Feb 15, 2024

I made two tiny PRs for the two general purpose lemmas, so those can be handled independently. I'll delete them from this one when done, but I don't think it blocks reviewing.

  1. [Merged by Bors] - feat: Add Real.pi_nonneg = Real.pi_pos.le #10596
  2. [Merged by Bors] - feat: Add mul_inv and inv_mul versions of div_le_one_of_le #10597

Copy link
Copy Markdown
Contributor

@Ruben-VandeVelde Ruben-VandeVelde left a comment

Choose a reason for hiding this comment

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

Neat. A few comments below

Copy link
Copy Markdown
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

A review of the module docstring.

@girving
Copy link
Copy Markdown
Collaborator Author

girving commented Feb 19, 2024

Thank for all the comments! Will reply to them soon, but wanted to leave a separate thought here before I forget it.

Currently one has to manually map each lemma to the appropriate rule type and priority, but I think this could be mostly automated:

  1. Have a single bound attribute that all the lemmas are registered to.
  2. For each, count the number of (1) general inequality hypotheses and (2) positivity/nonnegativity hypotheses, then map to priority based on lexicographic ordering of (1) and (2).

Does that seem reasonable? I can make that refactoring if so, and it would make it way easier to register new bound lemmas.

@ericrbg
Copy link
Copy Markdown
Contributor

ericrbg commented Feb 19, 2024

If this is your manual algorithm wrt applying the priorities to these lemmas, I think this would be most welcome!

@girving
Copy link
Copy Markdown
Collaborator Author

girving commented Feb 20, 2024

Priority selection is automatic now (see Bound/Attribute.lean for the algorithm), which makes registering lemmas way cleaner. The algorithm works well (I visually inspected everything it output, then confirmed that all of https://github.com/girving/ray works with the new algorithm), but it is arguably missing some whnf calls (basically, it assumes everything is already in sufficiently normal form). Advice on whether I should clean it further is appreciated.

This will make it way easier to register lemmas downstream as well, as one doesn't have to think about priorities.

@YaelDillies
Copy link
Copy Markdown
Contributor

It has significant overlap with positivity

Can it in fact not call positivity?

@girving
Copy link
Copy Markdown
Collaborator Author

girving commented Feb 23, 2024

It can call positivity as a closing tactic, and happy to add that if it seems reasonable, but this can do positivity goals that positivity cannot (in particular 0 < b - a). So I wouldn’t want to descend to that for all nonnegativity and positivity goals.

@YaelDillies
Copy link
Copy Markdown
Contributor

I argued before that positivity should know that a < b → 0 < b - a, so it really feels like you could plug bound on positivity by adding a parameter to positivity calls to use gcongr-like behavior.

Basically I'm trying to decide whether "Run positivity and gcongr intertwiningly" is a reasonable algorithm for bound. Because if it is that means we don't need to maintain an extra attribute for bound.

@girving
Copy link
Copy Markdown
Collaborator Author

girving commented Feb 24, 2024

bound is slightly more general than positivity and gcongr interleaved, since it also knows how to go from 0 <= a <= b to a / b <= 1 for each.

But if we could extend to handle that kind of thing too, I’d be open to that alternative. My sense from the thread linked in the original post is that there isn’t interest in broadening positivity and gcongr, though.

@YaelDillies
Copy link
Copy Markdown
Contributor

I just think it would be a shame to have three tools designed to solve basically the same problems.

@girving
Copy link
Copy Markdown
Collaborator Author

girving commented Feb 24, 2024

What’s the standard method of establishing consensus, given that this doesn’t seem to be general current view? (E.g., extending positivity further.)

@YaelDillies
Copy link
Copy Markdown
Contributor

Talk to Zulip, open an outstandingly good PR, run performance tests...

@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 6, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 22, 2024

PR summary 6aff75ed71

Import changes for modified files

Dependency changes

File Base Count Head Count Change
Mathlib.Tactic 2144 2148 +4 (+0.19%)
Import changes for all files
Files Import difference
There are 2588 files with changed transitive imports: this is too many to display!

Declarations diff

+ Bound.div_lt_one_of_pos_of_lt
+ Bound.le_self_pow_of_pos
+ Bound.one_lt_div_of_pos_of_lt
+ Bound.pow_le_pow_right_of_le_one_or_one_le
+ Nat.cast_pos_of_pos
+ Nat.one_le_cast_of_le
+ as
+ boundConfig
+ boundLinarith
+ boundNormNum
+ declPriority
+ ineqPriority
+ isZero
+ le_max_of_le_left_or_le_right
+ lt_max_of_lt_left_or_lt_right
+ min_le_of_left_le_or_right_le
+ min_lt_of_left_lt_or_right_lt
+ mul_lt_mul_left_of_pos_of_lt
+ mul_lt_mul_right_of_pos_of_lt
+ rpow_le_rpow_of_exponent_le_or_ge
+ scoreToConfig
+ test_unknown_identifier
+ typePriority
++ le_sqr_add

You can run this locally as follows
## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/declarations_diff.sh long <optional_commit>

The doc-module for script/declarations_diff.sh contains some details about this script.

@ghost ghost added the delegated This pull request has been delegated to the PR author (or occasionally another non-maintainer). label Aug 6, 2024
girving and others added 5 commits August 6, 2024 21:28
@girving
Copy link
Copy Markdown
Collaborator Author

girving commented Aug 6, 2024

How do I kick the CI so that it runs again? This seems transient:

Warning: some files were not found in the cache.
This usually means that your local checkout of mathlib4 has diverged from upstream.
If you push your commits to a branch of the mathlib4 repository, CI will build the oleans and they will be available later.
OpenSSL SSL_connect: Connection reset by peer in connection to lakecache.blob.core.windows.net:443 
2 download(s) failed
Error: Process completed with exit code 1.

@girving
Copy link
Copy Markdown
Collaborator Author

girving commented Aug 6, 2024

@j-loreaux What's the next step? Should I still wait for the remaining two reviewers?

@adomani
Copy link
Copy Markdown
Contributor

adomani commented Aug 6, 2024

Now that you've been delegated, you should feel free to merge! In any case, I'm happy with the current status!

🎉

@j-loreaux
Copy link
Copy Markdown
Contributor

I think things are okay. Kyle didn't revoke my delegation, so you could merge. But since I'm here:

bors merge

mathlib-bors bot pushed a commit that referenced this pull request Aug 6, 2024
#10562)

`bound` proves inequalities that follow "obviously by structure", assuming nonnegativity or positivity of subterms where necessary.  It is built on top of Aesop.  It has significant overlap with `positivity` and `gcongr`, but can also switch back and forth between those modes (such as when proving `0 < a * b - a * c` from `0 < a`, `c < b`).

Zulip discussion: https://leanprover.zulipchat.com/#narrow/stream/239415-metaprogramming-.2F-tactics/topic/.60bound.60.20mixes.20.60gcongr.60.20and.20.60positivity.60/near/412120380
@j-loreaux
Copy link
Copy Markdown
Contributor

bors r-

@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Aug 6, 2024

Canceled.

@j-loreaux
Copy link
Copy Markdown
Contributor

Oops, I thought CI was successful?

@girving
Copy link
Copy Markdown
Collaborator Author

girving commented Aug 6, 2024

Sorry, I'd missed that the attribute test was broken by the namespace change. Fixed.

@girving
Copy link
Copy Markdown
Collaborator Author

girving commented Aug 6, 2024

@j-loreaux You up for one more merge try? I think I can't do it, since the delegation went away with the new commit.

@kmill
Copy link
Copy Markdown
Contributor

kmill commented Aug 6, 2024

You should still have it delegated to you; adding commits doesn't remove that as far as I know. Maybe a bors r- might? But I don't think so. I'm happy to merge though.

bors r+

@github-actions github-actions bot added the ready-to-merge This PR has been sent to bors. label Aug 6, 2024
mathlib-bors bot pushed a commit that referenced this pull request Aug 6, 2024
#10562)

`bound` proves inequalities that follow "obviously by structure", assuming nonnegativity or positivity of subterms where necessary.  It is built on top of Aesop.  It has significant overlap with `positivity` and `gcongr`, but can also switch back and forth between those modes (such as when proving `0 < a * b - a * c` from `0 < a`, `c < b`).

Zulip discussion: https://leanprover.zulipchat.com/#narrow/stream/239415-metaprogramming-.2F-tactics/topic/.60bound.60.20mixes.20.60gcongr.60.20and.20.60positivity.60/near/412120380
@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Aug 6, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title feat(Tactic.Bound): Add a bound tactic for inequalities by structure [Merged by Bors] - feat(Tactic.Bound): Add a bound tactic for inequalities by structure Aug 6, 2024
@mathlib-bors mathlib-bors bot closed this Aug 6, 2024
@mathlib-bors mathlib-bors bot deleted the GI_bound branch August 6, 2024 23:54
@girving
Copy link
Copy Markdown
Collaborator Author

girving commented Aug 7, 2024

Woohoo, thank you!

bjoernkjoshanssen pushed a commit that referenced this pull request Sep 9, 2024
#10562)

`bound` proves inequalities that follow "obviously by structure", assuming nonnegativity or positivity of subterms where necessary.  It is built on top of Aesop.  It has significant overlap with `positivity` and `gcongr`, but can also switch back and forth between those modes (such as when proving `0 < a * b - a * c` from `0 < a`, `c < b`).

Zulip discussion: https://leanprover.zulipchat.com/#narrow/stream/239415-metaprogramming-.2F-tactics/topic/.60bound.60.20mixes.20.60gcongr.60.20and.20.60positivity.60/near/412120380
bjoernkjoshanssen pushed a commit that referenced this pull request Sep 9, 2024
#10562)

`bound` proves inequalities that follow "obviously by structure", assuming nonnegativity or positivity of subterms where necessary.  It is built on top of Aesop.  It has significant overlap with `positivity` and `gcongr`, but can also switch back and forth between those modes (such as when proving `0 < a * b - a * c` from `0 < a`, `c < b`).

Zulip discussion: https://leanprover.zulipchat.com/#narrow/stream/239415-metaprogramming-.2F-tactics/topic/.60bound.60.20mixes.20.60gcongr.60.20and.20.60positivity.60/near/412120380
bjoernkjoshanssen pushed a commit that referenced this pull request Sep 12, 2024
#10562)

`bound` proves inequalities that follow "obviously by structure", assuming nonnegativity or positivity of subterms where necessary.  It is built on top of Aesop.  It has significant overlap with `positivity` and `gcongr`, but can also switch back and forth between those modes (such as when proving `0 < a * b - a * c` from `0 < a`, `c < b`).

Zulip discussion: https://leanprover.zulipchat.com/#narrow/stream/239415-metaprogramming-.2F-tactics/topic/.60bound.60.20mixes.20.60gcongr.60.20and.20.60positivity.60/near/412120380
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

delegated This pull request has been delegated to the PR author (or occasionally another non-maintainer). ready-to-merge This PR has been sent to bors. t-meta Tactics, attributes or user commands

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants