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): use ratfunc #10421

Closed
wants to merge 1 commit into from

Conversation

ericrbg
Copy link
Collaborator

@ericrbg ericrbg commented Nov 23, 2021


Open in Gitpod

welcome to the most work anyone has ever done to remove a nontrivial on an unused lemma,,,

@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 Nov 23, 2021
@ericrbg ericrbg changed the title chore(ring_theory/polynomial/cyclotomic): remove a nontrivial chore(ring_theory/polynomial/cyclotomic): remove arguments Nov 23, 2021
@ericrbg
Copy link
Collaborator Author

ericrbg commented Nov 23, 2021

the build seems to be working but I still get an X :( ignore github, CI is fine with this

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. Using ratfunc R rather than a parameter K is slightly less general, however it does look nicer. So I'll let you decide whether to make this change.

Comment on lines 414 to 417
lemma cyclotomic_eq_prod_X_pow_sub_one_pow_moebius {n : ℕ} (R : Type*) [comm_ring R]
{K : Type*} [field K] [algebra (polynomial R) K] [is_fraction_ring (polynomial R) K] :
algebra_map _ K (cyclotomic n R) =
∏ i in n.divisors_antidiagonal, (algebra_map (polynomial R) K (X ^ i.snd - 1)) ^ μ i.fst :=
Copy link
Collaborator

Choose a reason for hiding this comment

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

I had to generalize ratfunc K to also accept is_domain K, not just field K: #10428. But then it works for free by replacing K with ratfunc K:

Suggested change
lemma cyclotomic_eq_prod_X_pow_sub_one_pow_moebius {n : ℕ} (R : Type*) [comm_ring R]
{K : Type*} [field K] [algebra (polynomial R) K] [is_fraction_ring (polynomial R) K] :
algebra_map _ K (cyclotomic n R) =
∏ i in n.divisors_antidiagonal, (algebra_map (polynomial R) K (X ^ i.snd - 1)) ^ μ i.fst :=
lemma cyclotomic_eq_prod_X_pow_sub_one_pow_moebius {n : ℕ} (R : Type*) [comm_ring R] [is_domain R] :
algebra_map _ (ratfunc R) (cyclotomic n R) =
∏ i in n.divisors_antidiagonal, (algebra_map (polynomial R) _ (X ^ i.snd - 1)) ^ μ i.fst :=

There are also some changed Ks below, but github doesn't allow me to make suggestions there...

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 think we should just close this PR and open a separate one; also, @riccardobrasca what do you think of this change?

Copy link
Member

Choose a reason for hiding this comment

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

LGTM too. Can we evaluate a rational function? If we already have that, the (currently not so nice) proof of polynomial.cyclotomic_coeff_zero should be golfed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, ratfunc.eval should work as expected (except when dividing by 0 of course!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can't golf the proof, sadly, as coeff_zero works for all comm_rings

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 we can prove it for Z and then use that cyclotomic n R comes from cyclotomic n Z.

Copy link
Collaborator Author

@ericrbg ericrbg Nov 24, 2021

Choose a reason for hiding this comment

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

I keep missing this and just saying "can't be done"! need to turn on my brain instead of just being in Lean mode all the time. Anyways, I currently have a sorry-free proof of ∀x, eval (cyclotomic n ℝ) x > 0 waiting for #10422, which is actually necessary for this proof (ratfunc.eval is only multiplicative if there isn't division by zero, and then we can split the product as required). There's also some issues with ratfunc ℤ.eval (namely that it doesn't work, and diamonds (seem to?) happen if you try generalize to [comm_ring K] [is_domain K]), but these can be easily dealt with by going to ℚ instead so there's no problems there.

Copy link
Member

Choose a reason for hiding this comment

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

Well, mathematically there is only cyclotomic n Z. In Lean it is convenient to have cyclotomic n R, but it comes from Z by definition. If something is wrong with integers coefficients we should fix it

Copy link
Member

Choose a reason for hiding this comment

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

So if the constant term of cyclotomic n Z is 1 the same is true for cyclotomic n R

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes I agree, I'm just laying out some of the issues that happen in Lean; there's nothing wrong with integer coeffs, just it's far more convenient to work over a field with ratfunc, and the suffices work out so that we do indeed get the result for all R.

src/ring_theory/polynomial/cyclotomic.lean Outdated Show resolved Hide resolved
bors bot pushed a commit that referenced this pull request Nov 23, 2021
…n K` (#10428)

Fixes one of the TODO's from the original ratfunc PR: apply all the easy generalizations where `K` doesn't need to be a field, only a `is_domain K` or even just `comm_ring K`.

This would allow us to use `ratfunc` in #10421.
@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 23, 2021
@ericrbg ericrbg force-pushed the nontriviality-mobius-inversion branch from fe59cba to 2f22001 Compare November 24, 2021 00:16
@ericrbg ericrbg changed the title chore(ring_theory/polynomial/cyclotomic): remove arguments chore(ring_theory/polynomial/cyclotomic): use ratfunc Nov 24, 2021
@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 24, 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 Nov 24, 2021
bors bot pushed a commit that referenced this pull request Nov 24, 2021
@bors
Copy link

bors bot commented Nov 24, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title chore(ring_theory/polynomial/cyclotomic): use ratfunc [Merged by Bors] - chore(ring_theory/polynomial/cyclotomic): use ratfunc Nov 24, 2021
@bors bors bot closed this Nov 24, 2021
@bors bors bot deleted the nontriviality-mobius-inversion branch November 24, 2021 08:49
ericrbg pushed a commit that referenced this pull request Nov 24, 2021
…n K` (#10428)

Fixes one of the TODO's from the original ratfunc PR: apply all the easy generalizations where `K` doesn't need to be a field, only a `is_domain K` or even just `comm_ring K`.

This would allow us to use `ratfunc` in #10421.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy < 20s of review time. See the lifecycle page for guidelines. 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