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(data/fintype/basic): add card_subtype_mono #14645

Closed
wants to merge 2 commits into from

Conversation

linesthatinterlace
Copy link
Collaborator

@linesthatinterlace linesthatinterlace commented Jun 9, 2022

This lemma naturally forms a counterpart to existing lemmas.

I've also renamed a lemma it uses that didn't seem to fit the existing naming pattern.


We were missing this which was a fairly straightforward lemma, so I added it (and edited a lemma name that looked misnamed).

@linesthatinterlace linesthatinterlace changed the title Card subtype le of imp feat(library): add card_subtype_le_of_imp Jun 9, 2022
@linesthatinterlace linesthatinterlace added awaiting-review The author would like community review of the PR awaiting-CI The author would like to see what CI has to say before doing more work. labels Jun 9, 2022
@eric-wieser
Copy link
Member

Note that if you repeat the PR title in the PR description, it will appear in the commit message twice (so don't do that!)

@@ -1456,6 +1456,11 @@ begin
intro; simp only [set.mem_to_finset, set.mem_compl_eq]; refl,
end

theorem card_subtype_le_of_imp (p q : α → Prop) (h : p ≤ q)
[fintype {x // p x}] [fintype {x // q x}] :
fintype.card {x // p x} ≤ fintype.card {x // q x} :=
Copy link
Member

Choose a reason for hiding this comment

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

Do we have the cardinal.mk version of this already?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, it's cardinal.mk_subtype_mono

So I think this should be called fintype.card_subtype_mono to match.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This makes sense, though I wonder if that means that subtype.imp_embedding should also be changed to something like subtype.embedding_mono?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, or indeed subtype_embedding_mono?

Copy link
Member

Choose a reason for hiding this comment

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

This lemma is called mono because it has a both in its conclusion and its aassumptions. subtype.imp_embedding does not match this pattern.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, that makes sense. Do you agree with my renaming of subtype.imp_embedding to subtype_imp_embedding? I couldn't work out why it was as it was.

Copy link
Member

Choose a reason for hiding this comment

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

I think the old name was probably better; it matches things like subtype.coind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm - but there are a number of subtype_ lemmas also. What's the rationale for why one is one and not the other?

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 not really sure, but in situations like this where things are already inconsistent, renaming just one lemma is just noise which might makes things better or might make things worse; so better to stick with the status quo and not repaint fractional bikesheds.

You could start a zulip thread, but I think this is one of those cases where no one really cares, and also where your time would be much better invested into developing your crypto / hamming stuff

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've removed the name change you thought wasn't a good idea, and done the one that you said was.

@github-actions github-actions bot removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Jun 9, 2022
@bryangingechen bryangingechen changed the title feat(library): add card_subtype_le_of_imp feat(data/fintype/basic.lean): add card_subtype_le_of_imp Jun 9, 2022
@bryangingechen bryangingechen changed the title feat(data/fintype/basic.lean): add card_subtype_le_of_imp feat(data/fintype/basic): add card_subtype_le_of_imp Jun 9, 2022
@linesthatinterlace linesthatinterlace changed the title feat(data/fintype/basic): add card_subtype_le_of_imp feat(data/fintype/basic): add card_subtype_mono Jun 9, 2022
@linesthatinterlace linesthatinterlace added the awaiting-CI The author would like to see what CI has to say before doing more work. label Jun 9, 2022
@eric-wieser
Copy link
Member

bors d+

Thanks!

@bors
Copy link

bors bot commented Jun 9, 2022

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

@github-actions github-actions bot added delegated The PR author may merge after reviewing final suggestions. and removed awaiting-review The author would like community review of the PR labels Jun 9, 2022
@linesthatinterlace
Copy link
Collaborator Author

bors r+

@bors
Copy link

bors bot commented Jun 9, 2022

👎 Rejected by label

@github-actions github-actions bot removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Jun 9, 2022
@linesthatinterlace
Copy link
Collaborator Author

bors r+

bors bot pushed a commit that referenced this pull request Jun 10, 2022
This lemma naturally forms a counterpart to existing lemmas.

I've also renamed a lemma it uses that didn't seem to fit the existing naming pattern.
@bors
Copy link

bors bot commented Jun 10, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(data/fintype/basic): add card_subtype_mono [Merged by Bors] - feat(data/fintype/basic): add card_subtype_mono Jun 10, 2022
@bors bors bot closed this Jun 10, 2022
@bors bors bot deleted the card_subtype_le_of_imp branch June 10, 2022 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated The PR author may merge after reviewing final suggestions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants