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(ring_theory/polynomial/cyclotomic): fix namespacing #9116

Closed
wants to merge 2 commits into from

Conversation

ericrbg
Copy link
Collaborator

@ericrbg ericrbg commented Sep 9, 2021

@riccardobrasca told me I got it wrong, so I fixed it :)


Open in Gitpod

@ericrbg ericrbg added easy < 20s of review time. See the lifecycle page for guidelines. awaiting-review The author would like community review of the PR labels Sep 9, 2021
@riccardobrasca
Copy link
Member

I always do mistakes with the name convention, so I will let someone else judge this, but those I am not sure about are polynomial.coprime_of_root_cyclotomic, polynomial.order_of_root_cyclotomic_dvd, polynomial.order_of_root_cyclotomic and polynomial.minpoly_primitive_root_dvd_cyclotomic .

Note that even if they concerns roots of unity in any field, it's better to have them here, since they need cyclotomic polyomials, over , whose definition uses the complex numbers.

@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 Sep 10, 2021
@semorrison semorrison removed the easy < 20s of review time. See the lifecycle page for guidelines. label Sep 11, 2021
@ericrbg
Copy link
Collaborator Author

ericrbg commented Sep 12, 2021

Thanks, I switched it to the shorter named and changed order_of_root_cyclotomic to order_of_root_cyclotomic_eq. For a separate PR, Riccardo, is there any specific reason that the last few results are specialized to ℤ? Is it just because everything is a ℤ-algebra, so it doesn't particularly matter? I'm glad to put in the effort generalising if not.

@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 Sep 12, 2021
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.

Thanks 🎉

bors merge

@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 13, 2021
@jcommelin
Copy link
Member

Thanks, I switched it to the shorter named and changed order_of_root_cyclotomic to order_of_root_cyclotomic_eq. For a separate PR, Riccardo, is there any specific reason that the last few results are specialized to ℤ? Is it just because everything is a ℤ-algebra, so it doesn't particularly matter? I'm glad to put in the effort generalising if not.

@ericrbg The statement cyclotomic n R = minpoly R \mu is not true for general R. So the statement would become more complicated. I think it's fine to keep \Z there.

bors bot pushed a commit that referenced this pull request Sep 13, 2021
@riccardobrasca told me I got it wrong, so I fixed it :)



Co-authored-by: Eric <ericrboidi@gmail.com>
@bors
Copy link

bors bot commented Sep 13, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title chore(ring_theory/polynomial/cyclotomic): fix namespacing [Merged by Bors] - chore(ring_theory/polynomial/cyclotomic): fix namespacing Sep 13, 2021
@bors bors bot closed this Sep 13, 2021
@bors bors bot deleted the ericrbg-patch-1 branch September 13, 2021 08:03
@riccardobrasca
Copy link
Member

As Johan said, some statements are not true in general, and I am not sure they are easy to generalize.

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

4 participants