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(ring_theory/unique_factorization_domain): unique_factorization_monoid structure on polynomials over ufd #4774

Closed
wants to merge 19 commits into from

Conversation

awainverse
Copy link
Collaborator

@awainverse awainverse commented Oct 24, 2020

Provides the unique_factorization_monoid structure on polynomials over a UFD


@awainverse awainverse added WIP Work in progress awaiting-review The author would like community review of the PR blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. labels Oct 24, 2020
@jcommelin
Copy link
Member

Wow, that was faster than I expected 🐙

@awainverse
Copy link
Collaborator Author

Does anyone know why this is causing class-instance depth problems, and how to avoid them?

@awainverse awainverse linked an issue Oct 24, 2020 that may be closed by this pull request
@awainverse
Copy link
Collaborator Author

awainverse commented Oct 24, 2020

Fixed the major typo.

@hrmacbeth
Copy link
Member

Can you please update docs/undergrad.yaml and docs/overview.yaml?

@awainverse
Copy link
Collaborator Author

I've added some entries to undergrad.yaml and overview.yaml.

@awainverse awainverse removed the WIP Work in progress label Oct 25, 2020
src/ring_theory/polynomial/basic.lean Outdated Show resolved Hide resolved
src/ring_theory/polynomial/basic.lean Outdated Show resolved Hide resolved
@awainverse
Copy link
Collaborator Author

awainverse commented Oct 28, 2020

It seems #4772 merged a bit weirdly (@bryangingechen would understand...) so the diff probably appears larger than it really is?

@jcommelin
Copy link
Member

@awainverse I'm nevertheless confused by the diff. Some of this stuff should now already be in mathlib, right?

@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 Oct 29, 2020
@awainverse
Copy link
Collaborator Author

Yes, all the stuff from #4772 is already in mathlib, but still shows up as new changes in the diff.
@bryangingechen closed that PR, pointing out that "This was merged in 28cc74f".
Apparently this was the problem: bors-ng/bors-ng#1067
I'm still not sure how to update the diff here. I've already merged in master since the merge.

@awainverse awainverse 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 Oct 29, 2020
@bryangingechen
Copy link
Collaborator

I think I understand why the diff isn't correct. If I navigate to your most recent merge commit and then click on "View Diff", I get this page: 3f777ea

This shows that the two parents of that commit are 1f0ddac and e8f8de6. The former is the previous commit on this branch and the latter is indeed a commit from master; however, that commit does not include the commit corresponding to #4772 which is 28cc74f. If I browse the repo at that commit and look at the history, I see that e8f8de6 is 5 commits before 28cc74f.

In short, to get a good diff, you'll have to merge a commit from master which includes 28cc74f.

@awainverse
Copy link
Collaborator Author

Ok, I merged master again, and it worked. I'm not sure why my previous merge was too early.

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

@bors
Copy link

bors bot commented Oct 29, 2020

👎 Rejected by label

@jcommelin jcommelin removed the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Oct 29, 2020
@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 Oct 29, 2020
@bryangingechen
Copy link
Collaborator

bors r+

bors bot pushed a commit that referenced this pull request Oct 29, 2020
…monoid` structure on polynomials over ufd (#4774)

Provides the `unique_factorization_monoid` structure on polynomials over a UFD
@bors
Copy link

bors bot commented Oct 29, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(ring_theory/unique_factorization_domain): unique_factorization_monoid structure on polynomials over ufd [Merged by Bors] - feat(ring_theory/unique_factorization_domain): unique_factorization_monoid structure on polynomials over ufd Oct 29, 2020
@bors bors bot closed this Oct 29, 2020
@bors bors bot deleted the polynomial_ufm branch October 29, 2020 21:58
lecopivo pushed a commit to lecopivo/mathlib that referenced this pull request Oct 31, 2020
…monoid` structure on polynomials over ufd (leanprover-community#4774)

Provides the `unique_factorization_monoid` structure on polynomials over a UFD
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.

unique factorisation of polynomials over a UFD
4 participants