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] - feat(logic/basic): Add forall_apply_eq_imp_iff #4109

Closed
wants to merge 5 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Sep 11, 2020

Also adds forall_apply_eq_imp_iff' for swapped forall arguments

This means that forall_range_iff can now be solved by simp.

This requires changes in data/pfun and measure_theory/borel_space, where non-terminal simps broke.


Discussed at https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/Should.20.60set.2Eforall_range_iff.60.20be.20taged.20.60simp.60.3F

@eric-wieser eric-wieser marked this pull request as ready for review September 11, 2020 11:00
@eric-wieser eric-wieser added the awaiting-review The author would like community review of the PR label Sep 11, 2020
@jcommelin jcommelin changed the title chore(data/set/basic): Mark forall_range_iff and exist_range_iff with simp chore(data/set/basic): tag forall_range_iff and exist_range_iff with simp Sep 11, 2020
@jcommelin
Copy link
Member

LGTM, let's see if the simp linter is happy with this.

@jcommelin jcommelin 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 Sep 11, 2020
@jcommelin
Copy link
Member

@eric-wieser CI doesn't like this 😢
It's up to you whether you want to fix mathlib or abandon this PR.

Comment on lines 1443 to 1444
@[simp] theorem forall_range_iff {p : α → Prop} : (∀ a ∈ range f, p a) ↔ (∀ i, p (f i)) :=
⟨assume h i, h (f i) (mem_range_self _), assume h a ⟨i, (hi : f i = a)⟩, hi ▸ h i⟩
Copy link
Member Author

Choose a reason for hiding this comment

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

I think I can fix this with

Suggested change
@[simp] theorem forall_range_iff {p : α → Prop} : (∀ a ∈ range f, p a) ↔ (∀ i, p (f i)) :=
⟨assume h i, h (f i) (mem_range_self _), assume h a ⟨i, (hi : f i = a)⟩, hi ▸ h i⟩
@[simp] theorem forall_range_iff_aux {p : α → Prop} : (∀ a i, f i = a → p a) ↔ (∀ i, p (f i)) :=
⟨assume h i, h (f i) i rfl, assume h a i hi, hi ▸ h i⟩
theorem forall_range_iff {p : α → Prop} : (∀ a ∈ range f, p a) ↔ (∀ i, p (f i)) :=
by simp

What would be a better name for forall_range_iff_aux?

Copy link
Member

Choose a reason for hiding this comment

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

The best I can come up with is forall_apply_eq_imp_iff. And it certainly is a good simp lemma 😀

Copy link
Member Author

Choose a reason for hiding this comment

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

If if helps, parts of it look like forall_eq', and the lemma used to prove exists_range_iff is exists_exists_eq_and.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also happy with forall_range_iff'. Whatever you want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, added using your suggested name.

(∀ a, ∀ b, f a = b → p b) ↔ (∀ a, p (f a)) :=
⟨λ h i, h i (f i) rfl, λ h i a hi, hi ▸ h i⟩

@[simp] theorem forall_apply_eq_imp_iff' {f : α → β} {p : β → Prop} :
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's less likely to show up, but b = f a could also be a pattern: forall_eq_apply_iff?
Do you want to add those?

Feel free to merge if the linter is happy

bors d+

Copy link
Member Author

Choose a reason for hiding this comment

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

Tempting, yeah. Is there a way for me to run the simp linter without waiting for a full mathlib build? The CI is taking a very long time on this PR, thanks to touching such low-level files I guess.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can (temporarily) add #lint at the bottom of the file, but it's not completely foolproof since simp lemmas in later files may change what the simp normal form is. I guess that's not likely for stuff in logic.basic though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given how slow the build was for this one, I think I'll come back and add the reversed-equality lemmas in a follow-up PR.

@bors
Copy link

bors bot commented Sep 11, 2020

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

by simp [@eq_comm _ a']
@[simp] theorem forall_apply_eq_imp_iff {f : α → β} {p : β → Prop} :
(∀ a, ∀ b, f a = b → p b) ↔ (∀ a, p (f a)) :=
⟨λ h a, h a (f a) rfl, λ h a b hab, hab ▸ h a⟩
Copy link
Member Author

Choose a reason for hiding this comment

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

Would by tidy be better or worse here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Worse, but I'm almost certain that tactic.tidy depends on this file anyways.

@@ -612,11 +612,7 @@ rel.core_inter _ s t

lemma mem_core_res (f : α → β) (s : set α) (t : set β) (x : α) :
x ∈ core (res f s) t ↔ (x ∈ s → f x ∈ t) :=
begin
simp [mem_core, mem_res], split,
Copy link
Member Author

Choose a reason for hiding this comment

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

Non-terminal simp is to blame here, but I suppose writing it this way lead to the proof being removable which is a good thing

@eric-wieser eric-wieser changed the title chore(data/set/basic): tag forall_range_iff and exist_range_iff with simp feat(logic/basic): Add forall_apply_eq_imp_iff Sep 11, 2020
@eric-wieser
Copy link
Member Author

More non-terminal simp breaking CI

@bryangingechen
Copy link
Collaborator

More non-terminal simp breaking CI

Yep, seems worth fixing.

Comment on lines 116 to 118
{ simp only [mem_range, exists_imp_distrib, forall_apply_eq_imp_iff'],
intro a,
exact generate_measurable.basic _ is_open_Iio }
Copy link
Member Author

Choose a reason for hiding this comment

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

I assume this is a reasonable fix, since I don't think I can rewrite this to put the simp last.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, I can eliminate this simp entirely

@eric-wieser
Copy link
Member Author

bors r+

@bors
Copy link

bors bot commented Sep 11, 2020

👎 Rejected by label

@eric-wieser
Copy link
Member Author

@jcommelin: Can I remove your changes requested label?

@bryangingechen bryangingechen removed the awaiting-author A reviewer has asked the author a question or requested changes label Sep 11, 2020
@bryangingechen
Copy link
Collaborator

@jcommelin: Can I remove your changes requested label?

Yes, and in the future feel free to do so without asking.

bors r+

@github-actions github-actions bot added the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Sep 11, 2020
bors bot pushed a commit that referenced this pull request Sep 11, 2020
Also adds forall_apply_eq_imp_iff' for swapped forall arguments

This means that `forall_range_iff` can now be solved by `simp`.

This requires changes in data/pfun and measure_theory/borel_space, where non-terminal `simp`s broke.
@bors
Copy link

bors bot commented Sep 11, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(logic/basic): Add forall_apply_eq_imp_iff [Merged by Bors] - feat(logic/basic): Add forall_apply_eq_imp_iff Sep 11, 2020
@bors bors bot closed this Sep 11, 2020
@bors bors bot deleted the eric-wieser/simp-range branch September 11, 2020 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants