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(algebra/module/basic): weaken assumption #16673

Closed
wants to merge 3 commits into from

Conversation

negiizhao
Copy link
Collaborator


Open in Gitpod

@negiizhao negiizhao added awaiting-review The author would like community review of the PR awaiting-CI The author would like to see what CI has to say before doing more work. t-algebra Algebra (groups, rings, fields etc) labels Sep 27, 2022
@github-actions github-actions bot removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Sep 27, 2022
@@ -617,7 +617,7 @@ end module

section division_ring

variables [division_ring R] [add_comm_group M] [module R M]
variables [division_ring R] [add_comm_monoid M] [module R M]
Copy link
Member

Choose a reason for hiding this comment

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

@YaelDillies, do we have division semirings now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yes, and it can even be [group_with_zero R] [add_monoid M] [distrib_mul_action R M]. Should we use this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The distinction between group_with_zero and division_semiring is irrelevant here currently I suspect, but we definitely want at least division_semiring to be able to apply it to nnreal and nnrat.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest to go with division_semiring, that is mathematically more meaningful.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see you already have the more general version. No worries!

bors d+

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't hurt to have the extra generality, but indeed I can't come up with an example where it would be useful.

Copy link
Member

Choose a reason for hiding this comment

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

You can maybe add a comment explaining that this applies to division_semiring, in particular nnreal.

@bors
Copy link

bors bot commented Sep 28, 2022

✌️ negiizhao can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@negiizhao
Copy link
Collaborator Author

bors r+


variables [division_ring R] [add_comm_group M] [module R M]
variables [group_with_zero R] [add_monoid M] [distrib_mul_action R M]
Copy link
Member

Choose a reason for hiding this comment

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

Presumably this no longer needs to be in this file now that it's not about module

bors r-

Copy link
Member

Choose a reason for hiding this comment

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

bors d+

@bors
Copy link

bors bot commented Sep 28, 2022

Canceled.

@bors
Copy link

bors bot commented Sep 28, 2022

✌️ negiizhao can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Nevermind, this is clearly in the right file already; sorry for kicking this off the queue too eagerly, and thanks for the generalization

bors merge

@leanprover-community-bot-assistant leanprover-community-bot-assistant 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 28, 2022
bors bot pushed a commit that referenced this pull request Sep 28, 2022
@bors
Copy link

bors bot commented Sep 29, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(algebra/module/basic): weaken assumption [Merged by Bors] - feat(algebra/module/basic): weaken assumption Sep 29, 2022
@bors bors bot closed this Sep 29, 2022
@bors bors bot deleted the FR_division_ring_to_no_zero_smul_divisors branch September 29, 2022 01:02
bors bot pushed a commit that referenced this pull request Sep 30, 2022
Split from #16412.

All of the following lemmas move from namespace `is_scalar_tower` to namespace `polynomial`.

Rename `aeval_apply` to `aeval_map_algebra_map` and swap the sides of the equality, remove `aeval_map` which is just the same as `aeval_map_algebra_map`.

Rename `algebra_map_aeval` to `aeval_algebra_map_apply` and swap the sides of the equality, rename original `aeval_algebra_map_apply` to `aeval_algebra_map_apply_eq_algebra_map_eval`. Make the new lemma a `simp` lemma and remove `simp` from the original one.

Rename `aeval_eq_zero_of_aeval_algebra_map_eq_zero` to `aeval_algebra_map_eq_zero_iff_of_injective` and make it an iff lemma.

Replace `aeval_eq_zero_of_aeval_algebra_map_eq_zero_field` by `aeval_algebra_map_eq_zero_iff`, it has weaker assumptions and is also an iff lemma.

- [x] depends on: #16673
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+`.) t-algebra Algebra (groups, rings, fields etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants