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(field_theory/adjoin): adjoining elements to fields #3913

Closed
wants to merge 32 commits into from

Conversation

tb65536
Copy link
Collaborator

@tb65536 tb65536 commented Aug 22, 2020

Defines adjoining elements to fields


We've proven the primitive element theorem, but here's a first chunk about defining adjoining elements to fields.

Defines adjoining elements to fields
@semorrison semorrison changed the title feat(field_theory/adjoin): new file feat(field_theory/adjoin): adjoining elements to fields Aug 23, 2020
@tb65536 tb65536 added the awaiting-review The author would like community review of the PR label Aug 23, 2020
@Vierkantor Vierkantor self-requested a review August 25, 2020 20:53
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.

Thanks for the PR 🎉! I would really like to see it merged. I'm afraid there is still quite some work to be done, though.

src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
@Vierkantor Vierkantor 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 Aug 25, 2020
tb65536 and others added 9 commits August 25, 2020 21:10
Co-authored-by: Vierkantor <Vierkantor@users.noreply.github.com>
Co-authored-by: Vierkantor <Vierkantor@users.noreply.github.com>
Co-authored-by: Vierkantor <Vierkantor@users.noreply.github.com>
Co-authored-by: Vierkantor <Vierkantor@users.noreply.github.com>
Co-authored-by: Vierkantor <Vierkantor@users.noreply.github.com>
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
Co-authored-by: Johan Commelin <johan@commelin.net>
@jcommelin
Copy link
Member

@tb65536 @pglutz This is starting to look good! If you can address the outstanding remarks, I guess this is close to getting merged.

@bryangingechen
Copy link
Collaborator

bryangingechen commented Aug 29, 2020

Looks like the F[(α)] notation somehow broke one of the linters?

edit: ah, this was already pointed out here.

edit 2: I made a Zulip thread.

Copy link
Collaborator

@bryangingechen bryangingechen left a comment

Choose a reason for hiding this comment

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

Just a few some minor style and formatting suggestions.

src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
src/field_theory/adjoin.lean Outdated Show resolved Hide resolved
tb65536 and others added 6 commits August 29, 2020 11:04
Co-authored-by: Bryan Gin-ge Chen <bryangingechen@gmail.com>
Co-authored-by: Bryan Gin-ge Chen <bryangingechen@gmail.com>
Co-authored-by: Bryan Gin-ge Chen <bryangingechen@gmail.com>
Co-authored-by: Bryan Gin-ge Chen <bryangingechen@gmail.com>
Co-authored-by: Bryan Gin-ge Chen <bryangingechen@gmail.com>
Co-authored-by: Bryan Gin-ge Chen <bryangingechen@gmail.com>
@Vierkantor
Copy link
Collaborator

Looks like the F[(α)] notation somehow broke one of the linters?

edit: ah, this was already pointed out here.

edit 2: I made a Zulip thread.

I figured out the error. It's not an error in a linter, but it's the parser getting confused by scripts/lint_mathlib.lean:88:

  [(linter_name, linter, (nolint_file.find linter_name).foldl rb_map.erase decls)]),

Replacing [( and )] with [ ( and ) ] fixes the issue, but it seems that we will need to change the syntax for field extension to avoid similar problems in the future.

@tb65536
Copy link
Collaborator Author

tb65536 commented Aug 30, 2020

Looks like the F[(α)] notation somehow broke one of the linters?
edit: ah, this was already pointed out here.
edit 2: I made a Zulip thread.

I figured out the error. It's not an error in a linter, but it's the parser getting confused by scripts/lint_mathlib.lean:88:

  [(linter_name, linter, (nolint_file.find linter_name).foldl rb_map.erase decls)]),

Replacing [( and )] with [ ( and ) ] fixes the issue, but it seems that we will need to change the syntax for field extension to avoid similar problems in the future.

Are you saying that we should change the field adjoin notation to F[ (a) ]?

@pglutz
Copy link
Collaborator

pglutz commented Aug 30, 2020

Looks like the F[(α)] notation somehow broke one of the linters?
edit: ah, this was already pointed out here.
edit 2: I made a Zulip thread.

I figured out the error. It's not an error in a linter, but it's the parser getting confused by scripts/lint_mathlib.lean:88:

  [(linter_name, linter, (nolint_file.find linter_name).foldl rb_map.erase decls)]),

Replacing [( and )] with [ ( and ) ] fixes the issue, but it seems that we will need to change the syntax for field extension to avoid similar problems in the future.

Are you saying that we should change the field adjoin notation to F[ (a) ]?

I kind of hate F[ (a) ] as notation. I propose that we pick one of the brackets here to replace [( )]. Perhaps we could use the flattened parentheses ⟮ ⟯ although there's some danger that people will get confused and think they are regular parentheses.

@tb65536 tb65536 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 Aug 30, 2020
@fpvandoorn
Copy link
Member

Yes, we should be careful combining multiple parentheses-like characters in a single token: that means that whenever those characters appear next to each other, they will never be interpreted as two parentheses-like characters anymore.
I'm much happier with the new parenthesis.

tb65536 and others added 2 commits August 30, 2020 14:42
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.

Looks good to me!

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.

Sorry for dropping out of the conversation. It looks good to me!

bors r+

@github-actions github-actions bot added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-review The author would like community review of the PR labels Sep 2, 2020
bors bot pushed a commit that referenced this pull request Sep 2, 2020
Defines adjoining elements to fields




Co-authored-by: tb65536 <tb65536@users.noreply.github.com>
Co-authored-by: Patrick Lutz <pglutz@berkeley.edu>
@bors
Copy link

bors bot commented Sep 2, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(field_theory/adjoin): adjoining elements to fields [Merged by Bors] - feat(field_theory/adjoin): adjoining elements to fields Sep 2, 2020
@bors bors bot closed this Sep 2, 2020
@bors bors bot deleted the field_adjoin branch September 2, 2020 09:42
PatrickMassot pushed a commit that referenced this pull request Sep 8, 2020
Defines adjoining elements to fields




Co-authored-by: tb65536 <tb65536@users.noreply.github.com>
Co-authored-by: Patrick Lutz <pglutz@berkeley.edu>
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

7 participants