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] - refactor(data/mv_polynomial/pderivative): make pderivative a linear map #4095

Closed
wants to merge 5 commits into from

Conversation

shingtaklam1324
Copy link
Collaborator

Make pderivative i a linear map as suggested at #4083 (comment)


Should I do the same to polynomial.derivative?

@shingtaklam1324 shingtaklam1324 added the awaiting-review The author would like community review of the PR label Sep 10, 2020
@shingtaklam1324
Copy link
Collaborator Author

shingtaklam1324 commented Sep 10, 2020

Actually, now I'm not sure about linear_map. This file currently works for all comm_semiring R, but the lemmas to do with mv_polynomial and smul all require comm_ring R. One example would be pderivative_C_mul, which would be linear_map.map_smul, except for some smul lemmas which are required (either smul_eq_C_mul or C_mul' would do).

Should I make R a comm_ring in pderivative.lean, or should I make pderivative an add_monoid_hom instead (and lose some of the properties), or should I try to prove some of the smul lemmas for comm_semiring R (if I can even prove them)?

Edit: Actually, smul_eq_C_mul's proof works with comm_semiring R. Perhaps some things in comm_ring.lean doesn't actually require comm_ring α

@bryangingechen
Copy link
Collaborator

Yes, I'd try and see if the mv_polynomial and smul lemmas you need can be generalized to comm_semiring. If you're feeling up to it, you could first make another PR that generalizes those files as much as possible and then mark this one as dependent on that.

@shingtaklam1324 shingtaklam1324 changed the title refactor(data/mv_polynomial/pderivative): make pderivative a linear map refactor(data/mv_polynomial/pderivative): make pderivative a linear map (deps: #4097) Sep 10, 2020
@shingtaklam1324 shingtaklam1324 added 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 Sep 10, 2020
@shingtaklam1324 shingtaklam1324 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 Sep 10, 2020
@shingtaklam1324 shingtaklam1324 changed the title refactor(data/mv_polynomial/pderivative): make pderivative a linear map (deps: #4097) refactor(data/mv_polynomial/pderivative): make pderivative a linear map Sep 10, 2020
@shingtaklam1324 shingtaklam1324 added the WIP Work in progress label Sep 10, 2020
@shingtaklam1324 shingtaklam1324 removed the WIP Work in progress label Sep 10, 2020
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 🎉 that's a nice cleanup!

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 11, 2020
bors bot pushed a commit that referenced this pull request Sep 11, 2020
@bors
Copy link

bors bot commented Sep 11, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(data/mv_polynomial/pderivative): make pderivative a linear map [Merged by Bors] - refactor(data/mv_polynomial/pderivative): make pderivative a linear map Sep 11, 2020
@bors bors bot closed this Sep 11, 2020
@bors bors bot deleted the pderivative-hom branch September 11, 2020 06:53
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

3 participants