Skip to content

Conversation

YaelDillies
Copy link
Collaborator

@YaelDillies YaelDillies commented Dec 1, 2024

The motivation for this change is that it is really confusing to run intro r s shouldnthaveintroedthat on a goal of the form ∀ r s : ℝ≥0∞, r ≤ s and get the nonsense-looking goal r = ↑shouldnthaveintroedthat → ∃ b : α, s = ↑b ∧ shouldnthaveintroedthat ≤ b⟩ instead of an error, and similarly when destructing something of the form ∃ r s : ℝ≥0∞, r < s.

Furthermore, I suspect this improves performance.


Open in Gitpod

Copy link

github-actions bot commented Dec 1, 2024

PR summary f1634b790d

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference

Declarations diff

+ LE
+ _root_.IsMax.withZero
+ instance (priority := 10) instLE : LE (WithBot α) where le := WithBot.LE
+ instance (priority := 10) instLE : LE (WithTop α) where le a b := WithBot.LE (α := αᵒᵈ) b a
+ instance (priority := 10) instLT : LT (WithBot α)
+ instance (priority := 10) instLT : LT (WithTop α)
+ instance (priority := 10) instLT : LT (WithZero α) := WithBot.instLT
+ instance (priority := 10) le : LE (WithZero α) := WithBot.instLE
+ instance : LE (Interval α) := WithBot.instLE
+ instance : OrderBot (Interval α) := WithBot.instOrderBot
+ le_unzero_iff
+ lt_coe_iff
+ lt_iff_exists_coe
+ lt_unzero_iff
+ nonpos_iff_eq_zero
+ pos_iff_ne_zero
+ unbot_le_iff
+ unbot_le_unbot
+ unbot_lt_unbot
+ unzero_lt_iff
++ le_iff_forall
++- instOrderBot
++- instPreorder
++--+- le_def
+-+ instBoundedOrder
+-+ instPartialOrder
+-+-+ le_coe_iff
+-+-+- coe_le_iff
- instance (priority := 10) le : LE (WithBot α)
- instance (priority := 10) le : LE (WithTop α)
- instance (priority := 10) le : LE (WithZero α)
- instance (priority := 10) lt : LT (WithBot α)
- instance (priority := 10) lt : LT (WithTop α)
- instance (priority := 10) lt : LT (WithZero α)
- instance : LE (Interval α) := WithBot.le
- instance : OrderBot (Interval α) := WithBot.orderBot
- orderBot
- orderTop
- partialOrder
- preorder
-+--++ lt_def

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.


No changes to technical debt.

You can run this locally as

./scripts/technical-debt-metrics.sh pr_summary
  • The relative value is the weighted sum of the differences with weight given by the inverse of the current value of the statistic.
  • The absolute value is the relative value divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).

@YaelDillies YaelDillies added WIP Work in progress t-order Order theory labels Dec 1, 2024
@YaelDillies YaelDillies force-pushed the with_bot_inductive_le branch 2 times, most recently from 9bcb538 to eee58b0 Compare December 7, 2024 15:43
@leanprover-community-bot-assistant leanprover-community-bot-assistant added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Dec 11, 2024
@YaelDillies YaelDillies force-pushed the with_bot_inductive_le branch from c55c9a0 to d62c102 Compare December 18, 2024 10:25
@leanprover-community-bot-assistant leanprover-community-bot-assistant removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Dec 18, 2024
@leanprover-community-bot-assistant leanprover-community-bot-assistant added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Dec 27, 2024
@YaelDillies YaelDillies force-pushed the with_bot_inductive_le branch 2 times, most recently from 790ee1d to 54d040e Compare December 29, 2024 09:44
@mathlib4-dependent-issues-bot mathlib4-dependent-issues-bot added the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Dec 29, 2024
@leanprover-community-bot-assistant leanprover-community-bot-assistant removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Dec 29, 2024
@YaelDillies YaelDillies force-pushed the with_bot_inductive_le branch from 54d040e to 2a0478e Compare December 29, 2024 22:03
@mathlib4-dependent-issues-bot mathlib4-dependent-issues-bot removed the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Jan 27, 2025
@leanprover-community-bot-assistant leanprover-community-bot-assistant added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Jan 27, 2025
@YaelDillies YaelDillies force-pushed the with_bot_inductive_le branch from 2a0478e to 3a90c63 Compare January 27, 2025 17:35
@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Jan 27, 2025
@mathlib4-dependent-issues-bot mathlib4-dependent-issues-bot added the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Jan 30, 2025
@mathlib4-dependent-issues-bot mathlib4-dependent-issues-bot removed the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Feb 12, 2025
@leanprover-community-bot-assistant leanprover-community-bot-assistant 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 12, 2025
@YaelDillies YaelDillies force-pushed the with_bot_inductive_le branch from 3a90c63 to 13ecad7 Compare February 12, 2025 13:41
@github-actions github-actions bot 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 12, 2025
@YaelDillies YaelDillies force-pushed the with_bot_inductive_le branch from 13ecad7 to dca366e Compare February 20, 2025 00:15
@mathlib4-dependent-issues-bot mathlib4-dependent-issues-bot added the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Feb 20, 2025
@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Sep 15, 2025
@mathlib4-merge-conflict-bot mathlib4-merge-conflict-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Sep 15, 2025
@mathlib4-merge-conflict-bot
Copy link
Collaborator

This pull request has conflicts, please merge master and resolve them.

@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Sep 15, 2025
Copy link
Collaborator

@JovanGerb JovanGerb left a comment

Choose a reason for hiding this comment

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

How does this PR interact with the recent push by @kim-em to make WithBot and WithTop be the same inductive? Given what you said in your last message, I think it would be fitting to have the relation also be a shared inductive.

@YaelDillies
Copy link
Collaborator Author

How does this PR interact with the recent push by @kim-em to make WithBot and WithTop be the same inductive? Given what you said in your last message, I think it would be fitting to have the relation also be a shared inductive.

I was thinking the same, in fact. It's not my favorite solution, but I'll try it

@Vierkantor
Copy link
Contributor

I'm marking this PR as awaiting-author until you implement the shared inductive ≤ relation. Feel free to remove the label if you don't think this is a good idea anymore.

@Vierkantor Vierkantor added the awaiting-author A reviewer has asked the author a question or requested changes. label Sep 30, 2025
@YaelDillies YaelDillies force-pushed the with_bot_inductive_le branch from 7b40b78 to 0e9e5ab Compare October 5, 2025 12:53
@YaelDillies YaelDillies removed the awaiting-author A reviewer has asked the author a question or requested changes. label Oct 5, 2025
@JovanGerb
Copy link
Collaborator

Wouldn't le_iff_forall be a more sensible name compared to le_iff_exists?

/--
info: Try these:
• 🎉 finiteness
• 🎉 tauto_set
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks pretty undesirable to me: the test was meant to be showing that finiteness is suggested on this goal, and now tauto_set is suggested to show finiteness of 1?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In my opinion, hint is quite broken. It only suggests one thing and it doesn't have a way of telling which suggesting is better that which other suggestion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, unfortunately this test is not very deterministic and so I can't do much about it

Copy link
Contributor

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

I like the change! Now that most of the build issues and conflict with other PRs appear fixed, let's get this merged.

bors d+

@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Oct 15, 2025

✌️ YaelDillies can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@YaelDillies
Copy link
Collaborator Author

bors merge

mathlib-bors bot pushed a commit that referenced this pull request Oct 16, 2025
The motivation for this change is that it is really confusing to run `intro r s shouldnthaveintroedthat` on a goal of the form `∀ r s : ℝ≥0∞, r ≤ s` and get the nonsense-looking goal `r = ↑shouldnthaveintroedthat → ∃ b : α, s = ↑b ∧ shouldnthaveintroedthat ≤ b⟩` instead of an error, and similarly when destructing something of the form `∃ r s : ℝ≥0∞, r < s`.

Furthermore, I suspect this improves performance.
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Oct 16, 2025

Build failed:

@YaelDillies YaelDillies force-pushed the with_bot_inductive_le branch from 704c9d8 to dab529e Compare October 16, 2025 16:24
@YaelDillies
Copy link
Collaborator Author

bors merge

mathlib-bors bot pushed a commit that referenced this pull request Oct 16, 2025
The motivation for this change is that it is really confusing to run `intro r s shouldnthaveintroedthat` on a goal of the form `∀ r s : ℝ≥0∞, r ≤ s` and get the nonsense-looking goal `r = ↑shouldnthaveintroedthat → ∃ b : α, s = ↑b ∧ shouldnthaveintroedthat ≤ b⟩` instead of an error, and similarly when destructing something of the form `∃ r s : ℝ≥0∞, r < s`.

Furthermore, I suspect this improves performance.
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Oct 16, 2025

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title refactor: define /< on WithBot/WithTop by induction [Merged by Bors] - refactor: define /< on WithBot/WithTop by induction Oct 16, 2025
@mathlib-bors mathlib-bors bot closed this Oct 16, 2025
@mathlib-bors mathlib-bors bot deleted the with_bot_inductive_le branch October 16, 2025 17:14
@JovanGerb
Copy link
Collaborator

@YaelDillies, I'm a bit confused why you defined as an inductive predicate, but < using match. Are you going to define < as an inductive predicate in a future PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.