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 port: Data.Set.Lattice #1121

Closed
wants to merge 27 commits into from

Conversation

ChrisHughes24
Copy link
Member

@ChrisHughes24 ChrisHughes24 commented Dec 20, 2022

Mostly name changes. The proof of subset_bunionᵢ_of_mem needed a bunch of explicit arguments for some reason. I also moved the definition of set.sUnion from Init.Set into this file and changed its name. I also protected the definition of Set.compl

@ChrisHughes24 ChrisHughes24 added WIP Work in progress mathlib-port This is a port of a theory file from mathlib. labels Dec 20, 2022
@ChrisHughes24 ChrisHughes24 added awaiting-review The author would like community review of the PR and removed WIP Work in progress labels Dec 20, 2022
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Copy link
Member

@kbuzzard kbuzzard left a comment

Choose a reason for hiding this comment

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

A monumental effort! I approve modulo the things I flagged.

Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
#align set.sInter Set.interₛ

/-- Notation for `Set.interₛ` Intersection of a set of sets. -/
prefix:110 "⋂₀ " => interₛ
Copy link
Member

Choose a reason for hiding this comment

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

Talking point: how about we change this to ⋂ₛ?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, but maybe we do it after the port?

Copy link
Member

Choose a reason for hiding this comment

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

Part of me agrees, and part of me wants to say "well we just changed ten other things to use this little s and little i notation and I think it's really cool".

Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
Mathlib/Data/Set/Lattice.lean Outdated Show resolved Hide resolved
hf.infᵢ_nat_add k
#align antitone.Inter_nat_add Antitone.interᵢ_nat_add

--Porting note: removing `simp`. LHS does not simplify
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 confused about this one.

example (f : ℕ → Set α) (k : ℕ) :
    (⋃ n, ⋂ i ≥ n, f (i + k)) = ⋃ n, ⋂ i ≥ n, f i := by simp

works fine if unionᵢ_interᵢ_ge_nat_add is tagged @[simp] and doesn't work otherwise. Is this a bug in the linter? Are we sure this shouldn't be tagged @[simp]?

Copy link
Member Author

Choose a reason for hiding this comment

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

@semorrison semorrison 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 Dec 20, 2022
ChrisHughes24 and others added 6 commits December 21, 2022 09:53
Co-authored-by: Kevin Buzzard <k.buzzard@imperial.ac.uk>
Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com>
Co-authored-by: Kevin Buzzard <k.buzzard@imperial.ac.uk>
Co-authored-by: Kevin Buzzard <k.buzzard@imperial.ac.uk>
Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com>
Co-authored-by: Kevin Buzzard <k.buzzard@imperial.ac.uk>
Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com>
@ChrisHughes24 ChrisHughes24 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 Dec 21, 2022
@eric-wieser
Copy link
Member

eric-wieser commented Dec 21, 2022

This will conflict with @negiizhao's #1109, for which leanprover-community/mathlib#17882 is on the queue

@negiizhao
Copy link
Collaborator

I believe this just needs to remove some lemmas in Data.Set.Lattice.

@eric-wieser
Copy link
Member

That sounds like an argument in favor of merging #1109 first?

@negiizhao
Copy link
Collaborator

Both are easy to deal with. To ensure that no more lemmas are removed by mistake, it might be better to merge #1121 first (then we can compare the diffs).

@jcommelin
Copy link
Member

Let's merge this. Then we can move forward with #1109.
If @kbuzzard's question needs to be addressed, then that can be done in a future PR.

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 Dec 22, 2022
bors bot pushed a commit that referenced this pull request Dec 22, 2022
Mostly name changes. The proof of `subset_bunionᵢ_of_mem` needed a bunch of explicit arguments for some reason. I also moved the definition of `set.sUnion` from `Init.Set` into this file and changed its name. I also `protected` the definition of `Set.compl`

Co-authored-by: Chris Hughes <33847686+ChrisHughes24@users.noreply.github.com>
@bors
Copy link

bors bot commented Dec 22, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat port: Data.Set.Lattice [Merged by Bors] - feat port: Data.Set.Lattice Dec 22, 2022
@bors bors bot closed this Dec 22, 2022
@bors bors bot deleted the Data.Set.Lattice branch December 22, 2022 17:18
bors bot pushed a commit that referenced this pull request May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mathlib-port This is a port of a theory file from mathlib. 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

8 participants