-
Notifications
You must be signed in to change notification settings - Fork 295
[Merged by Bors] - refactor(data/set/finite): reorganize and put emphasis on fintype instances #14136
Conversation
src/combinatorics/configuration.lean
Outdated
have h2 : ∀ l : {l : L // p ∈ l}, fintype.card {q // q ∈ l.1 ∧ q ≠ p} = order P L, | ||
{ intro l, | ||
rw [←fintype.card_congr (equiv.subtype_subtype_equiv_subtype_inter _ _), | ||
fintype.card_subtype_compl, ←nat.card_eq_fintype_card], | ||
any_goals { apply_instance }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly what happened here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is because I changed card_subtype_compl
from
lemma fintype.card_subtype_compl [fintype α] {p : α → Prop} [decidable_pred p] :
fintype.card {x // ¬ p x} = fintype.card α - fintype.card {x // p x} :=
to
lemma fintype.card_subtype_compl [fintype α]
(p : α → Prop) [fintype {x // p x}] [fintype {x // ¬ p x}] :
fintype.card {x // ¬ p x} = fintype.card α - fintype.card {x // p x}
and the rewrite is somehow not able to find the third fintype instance. If you give it the predicate directly, then it works fine. In the commit I pushed, I modified it to give the predicate directly.
I'm not sure what the best thing to do here is, and I don't really understand what's causing the instance lookup failure... (Maybe it's ne
vs not-equals in whatever p
it infers?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take it on trust that the lemmas in data/set/finite
have moved and none have been lost, otherwise LGTM!
Co-authored-by: Bhavik Mehta <[email protected]>
@b-mehta Methodologically, I did copy/paste to put things into order, so nothing should be lost (and thankfully I haven't had to do a complicated merge so far). There's also a metaproof that nothing has gone missing: mathlib still compiles, and nobody adds anything to data/set/finite unless it's for something else! |
I had been planning on making |
[fintype s] [fintype t] : (s ∩ t).to_finset = s.to_finset ∩ t.to_finset := | ||
by { ext, simp } | ||
|
||
lemma to_finset_union {α : Type*} [decidable_eq α] (s t : set α) [fintype (s ∪ t : set α)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for some of these to be simp
and not others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, this is how they were, and I didn't want to see if mathlib would compile with them as simp
lemmas.
exact m } | ||
end | ||
end | ||
/-- A `fintype` structure on `insert a s` when inserting a new element. -/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reference fintype_insert
in this docstring (and the next), and also vice-versa?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do that in a followup PR
fintype.card_of_subsingleton _ | ||
/-- Normally, `insert a s` for `finset` requires `[decidable_eq α]`, but we can use this weaker | ||
assumption here. -/ | ||
instance fintype_insert' (a : α) (s : set α) [decidable $ a ∈ s] [fintype s] : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this instance as well as the previous seems a bit odd to me, but can we reference the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from the original fintype_insert
, which derives decidable membership from decidable equality and chooses one of the two fintype
instances based on that decision. The new fintype_insert
just uses the finset
insert, which makes it more like instances for other set operations -- having looked at the history, it looks like the reason for this is that the original set.finite
was a custom inductive type that made no use of finset
. I let the original fintype_insert
hang around in this form since it might still be useful.
src/data/fintype/basic.lean
Outdated
classical, | ||
rw [fintype.card_of_subtype (set.to_finset pᶜ), set.to_finset_compl p, finset.card_compl, | ||
fintype.card_of_subtype (set.to_finset p)]; | ||
intros; simp; refl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we missing a simp lemma?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is code that had previously been through review, but I went ahead and fixed the non-terminal simp
.
What's going on is that this proof is that p
is not a set
but the rewrites are using it as if it were one. In a quick test, set_of p
doesn't fix it.
inductive finite (s : set α) : Prop | ||
| intro : fintype s → finite | ||
|
||
lemma finite_def {s : set α} : finite s ↔ nonempty (fintype s) := ⟨λ ⟨h⟩, ⟨h⟩, λ ⟨h⟩, ⟨h⟩⟩ | ||
/-- Constructor for `set.finite` with the `fintype` as an instance argument. -/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make intro
be this with something like
inductive finite (s : set α) : Prop
| of_fintype [fintype s] : finite
Or is that annoying?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd found writing finite_def
awkward that way, so I figured I'd leave it as Yury wrote it.
Thanks 🎉 bors merge |
…tances (#14136) I went through `data/set/finite` and reorganized it by rough topic (and moved some lemmas to their proper homes; closes #11177). Two important parts of this module are (1) `fintype` instances for various set constructions and (2) ways to create `set.finite` terms. This change puts the module closer to following a design where the `set.finite` terms are created in a formulaic way from the `fintype` instances. One tool for this is a `set.finite_of_fintype` constructor, which lets typeclass inference do most of the work. Included in this commit is changing `set.infinite` to be protected so that it does not conflict with `infinite`.
Pull request successfully merged into master. Build succeeded: |
This follows up with some review comments for #14136.
I went through
data/set/finite
and reorganized it by rough topic (and moved some lemmas to their proper homes; closes #11177). Two important parts of this module are (1)fintype
instances for various set constructions and (2) ways to createset.finite
terms. This change puts the module closer to following a design where theset.finite
terms are created in a formulaic way from thefintype
instances. One tool for this is aset.finite_of_fintype
constructor, which lets typeclass inference do most of the work.Included in this commit is changing
set.infinite
to be protected so that it does not conflict withinfinite
.