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] - chore(field_theory/separable): generalize theorems #10337

Closed
wants to merge 17 commits into from

Conversation

ericrbg
Copy link
Collaborator

@ericrbg ericrbg commented Nov 15, 2021


Open in Gitpod

I touched a lot of adjacent files to fix proofs that got modified, sorry! Also, something that I wonder is whether it is worth making some of the char_p results about ring_char R instead; another thing I worry about is the fact I turned quite a few fact p.prime arguments into p≠0; in most cases, this was a direct improvement, but when it's char p, we already have p≠0↔p.prime, and so we lose some convenience by giving up typeclasses there. Is it worth reverting that? Or making primed versions that take the fact?

by_contra hzero,
exact hdiv ((zmod.nat_coe_zmod_eq_zero_iff_dvd n p).1 hzero) },
{ exact (separable_X_pow_sub_C 1
(λ h, hdiv $ (zmod.nat_coe_zmod_eq_zero_iff_dvd n p).1 h) one_ne_zero).squarefree },
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is some weird issues that happen if you leave in the original proof but with .squarefree instead; I'm happy to revert this lemma deletion if you guys think this is worth it.

@ericrbg ericrbg added the awaiting-CI The author would like to see what CI has to say before doing more work. label Nov 15, 2021
@@ -195,7 +195,8 @@ begin
letI := classical.dec_eq E,
rw [norm_gen_eq_prod_roots pb hE, fintype.prod_equiv pb.lift_equiv', finset.prod_mem_multiset,
finset.prod_eq_multiset_prod, multiset.to_finset_val,
multiset.erase_dup_eq_self.mpr (nodup_roots ((separable_map _).mpr hfx)), multiset.map_id],
multiset.erase_dup_eq_self.mpr, multiset.map_id],
{ exact nodup_roots ((separable_map _).mpr hfx) },
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'm not sure why this is necessary, nodup_roots only changed from field K to comm_ring R, is_domain R!

@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 Nov 15, 2021
@ericrbg ericrbg added the awaiting-review The author would like community review of the PR label Nov 16, 2021
src/field_theory/separable.lean Outdated Show resolved Hide resolved
src/field_theory/separable.lean Show resolved Hide resolved
src/field_theory/separable.lean Outdated Show resolved Hide resolved
variable [hp : fact p.prime]
include hp

theorem expand_char (f : polynomial R) : map (frobenius R p) (expand R p f) = f ^ p :=
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be called map_frobenius_expand? Or are you just moving this lemma?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

all the lemmas here (apart from separable_of_subsingleton) were just moved; I'm still happy to rename this, though; it seems a better name

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should go through the whole file in a separate PR since there's some suboptimal naming throughout e.g. the pattern whatever_of_separable to separable.whatever.

src/field_theory/separable.lean Show resolved Hide resolved
@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 Nov 16, 2021
ericrbg and others added 2 commits November 16, 2021 09:29
Co-authored-by: Johan Commelin <johan@commelin.net>
@ericrbg ericrbg 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 Nov 16, 2021
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Nov 17, 2021
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Nov 17, 2021
Copy link
Collaborator

@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.

LGTM. I'll have @jcommelin have the final say.

@bors
Copy link

bors bot commented Nov 23, 2021

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

Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

bors d+

@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 Nov 23, 2021
@ericrbg
Copy link
Collaborator Author

ericrbg commented Nov 23, 2021

bors r+

bors bot pushed a commit that referenced this pull request Nov 23, 2021


Co-authored-by: Eric <37984851+ericrbg@users.noreply.github.com>
@bors
Copy link

bors bot commented Nov 23, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title chore(field_theory/separable): generalize theorems [Merged by Bors] - chore(field_theory/separable): generalize theorems Nov 23, 2021
@bors bors bot closed this Nov 23, 2021
@bors bors bot deleted the separable-comm_rings branch November 23, 2021 13:11
ericrbg added a commit that referenced this pull request Nov 24, 2021


Co-authored-by: Eric <37984851+ericrbg@users.noreply.github.com>
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

4 participants