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(algebra/algebra/basic): replace algebra.comap with restrict_scalars #7949

Closed
wants to merge 20 commits into from

Conversation

hrmacbeth
Copy link
Member

@hrmacbeth hrmacbeth commented Jun 15, 2021

The constructions algebra.comap and restrict_scalars are essentially the same thing -- a type synonym to allow one to switch to a smaller scalar field. Previously restrict_scalars was for modules and algebra.comap for algebras; I am unifying them so that restrict_scalars works for both.

Declaration changes:

  • algebra.comap, algebra.comap.inhabited, is_scalar_tower.comap
    Use the pre-existing (for modules) restrict_scalars, restrict_scalars.inhabited, restrict_scalars.is_scalar_tower

  • algebra.comap.X for X in semiring, ring, comm_semiring, comm_ring, algebra
    Replaced with restrict_scalars.X

  • algebra.comap.algebra'
    Replaced with restrict_scalars.algebra_orig (to be consistent with restrict_scalars.module_orig)

  • algebra.comap.to_comap and algebra.comap.of_comap
    Combined into an alg_equiv and renamed restrict_scalars.alg_equiv (to be consistent with restrict_scalars.linear_equiv)

  • subalgebra.comap
    Replaced with a generalized version, subalgebra.restrict_scalars, which (to be consistent with submodule.restrict_scalars) applies to an is_scalar_tower, not just to the type synonym

Deleted altogether:

  • algebra.to_comap, algebra.to_comap_apply
    This construction is now
    (algebra.of_id S (restrict_scalars R S A)).restrict_scalars R
    It was only used once in mathlib, where I have replaced it by its definition

  • alg_hom.comap, alg_equiv.comap
    These are not currently used in mathlib but if needed one can instead use alg_hom.restrict_scalars and alg_equiv.restrict_scalars

  • is_scalar_tower.algebra_comap_eq
    The proof is now rfl and it was never used in mathlib.

It would then be possible, in a follow-up PR, to rename subalgebra.comap' to subalgebra.comap, although I have no immediate plans to do this.


Open in Gitpod

Zulip

@hrmacbeth hrmacbeth added the awaiting-review The author would like community review of the PR label Jun 15, 2021
@Vierkantor Vierkantor self-requested a review June 15, 2021 14:48
/-- Identity homomorphism `A →ₐ[S] restrict_scalars R S A`. -/
def restrict_scalars.to_restrict_scalars : A →ₐ[S] restrict_scalars R S A := alg_hom.id S A
/-- Identity homomorphism `restrict_scalars R S A →ₐ[S] A`. -/
def restrict_scalars.of_restrict_scalars : restrict_scalars R S A →ₐ[S] A := alg_hom.id S A
Copy link
Member

Choose a reason for hiding this comment

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

What's the linear map version of these functions called? It's not clear to me that the alg_hom versions are deserving of the shorter name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAICT, there was no linear map version, only algebra.comap.to_comap and algebra.to_comap which did different things annoyingly enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed to alg_equiv.self_to_restrict_scalars, feel free to rename.

Copy link
Member Author

Choose a reason for hiding this comment

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

@eric-wieser @Vierkantor I noticed that in #7807, the linear equiv version is introduced. It's named restrict_scalars.linear_equiv and goes in the opposite direction, A →ₐ[S] restrict_scalars R S A. So I have changed up the algebra equiv version again to mimick it.

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.

Thanks! Your choices for what to modify or delete look good to me, though some of the older stuff might be worth polishing as we update it.

Beware that there is a merge conflict with #7807, since both PRs touch the docstrings.

src/algebra/algebra/basic.lean Outdated Show resolved Hide resolved
src/algebra/algebra/basic.lean Outdated Show resolved Hide resolved
hrmacbeth and others added 2 commits June 15, 2021 11:38
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
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.

Thanks 🎉 Let's wait for everything to build successfully before merging.

bors d+

@bors
Copy link

bors bot commented Jun 15, 2021

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

@github-actions github-actions bot added delegated The PR author may merge after reviewing final suggestions. merge-conflict Please `git merge origin/master` then a bot will remove this label. and removed awaiting-review The author would like community review of the PR labels Jun 15, 2021
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Jun 15, 2021
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@hrmacbeth
Copy link
Member Author

bors r+

@github-actions github-actions bot added the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Jun 16, 2021
bors bot pushed a commit that referenced this pull request Jun 16, 2021
…ct_scalars` (#7949)

The constructions `algebra.comap` and `restrict_scalars` are essentially the same thing -- a type synonym to allow one to switch to a smaller scalar field.  Previously `restrict_scalars` was for modules and `algebra.comap` for algebras; I am unifying them so that `restrict_scalars` works for both.

Declaration changes:

- `algebra.comap`, `algebra.comap.inhabited`, `is_scalar_tower.comap`
Use the pre-existing (for modules) `restrict_scalars`, `restrict_scalars.inhabited`, `restrict_scalars.is_scalar_tower`

- `algebra.comap.X` for `X` in `semiring`, `ring`, `comm_semiring`, `comm_ring`, `algebra`
Replaced with `restrict_scalars.X`

- `algebra.comap.algebra'`
Replaced with `restrict_scalars.algebra_orig` (to be consistent with `restrict_scalars.module_orig`)

- `algebra.comap.to_comap` and `algebra.comap.of_comap`
Combined into an `alg_equiv` and renamed `restrict_scalars.alg_equiv` (to be consistent with `restrict_scalars.linear_equiv`)

- `subalgebra.comap`
Replaced with a generalized version, `subalgebra.restrict_scalars`, which (to be consistent with `submodule.restrict_scalars`) applies to an `is_scalar_tower`, not just to the type synonym

Deleted altogether:

- `algebra.to_comap`, `algebra.to_comap_apply`
This construction is now 
`(algebra.of_id S (restrict_scalars R S A)).restrict_scalars R`
It was only used once in mathlib, where I have replaced it by its definition

- `alg_hom.comap`, `alg_equiv.comap`
These are not currently used in mathlib but if needed one can instead use `alg_hom.restrict_scalars` and `alg_equiv.restrict_scalars`

- `is_scalar_tower.algebra_comap_eq`
The proof is now `rfl` and it was never used in mathlib.

It would then be possible, in a follow-up PR, to rename `subalgebra.comap'` to `subalgebra.comap`, although I have no immediate plans to do this.
@bors
Copy link

bors bot commented Jun 16, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(algebra/algebra/basic): replace algebra.comap with restrict_scalars [Merged by Bors] - refactor(algebra/algebra/basic): replace algebra.comap with restrict_scalars Jun 16, 2021
@bors bors bot closed this Jun 16, 2021
@bors bors bot deleted the remove-comap-algebra branch June 16, 2021 09:23
bors bot pushed a commit that referenced this pull request Sep 17, 2021
…`subalgebra.restrict_scalars` (#9251)

We use the name `restrict_scalars` everywhere else, so I kept that one instead of `res`.

`res` was here first, but the duplicate was added by #7949 presumably because the `res` name wasn't discoverable.
bors bot pushed a commit that referenced this pull request Sep 17, 2021
…`subalgebra.restrict_scalars` (#9251)

We use the name `restrict_scalars` everywhere else, so I kept that one instead of `res`.

`res` was here first, but the duplicate was added by #7949 presumably because the `res` name wasn't discoverable.
bors bot pushed a commit that referenced this pull request Sep 18, 2021
…`subalgebra.restrict_scalars` (#9251)

We use the name `restrict_scalars` everywhere else, so I kept that one instead of `res`.

`res` was here first, but the duplicate was added by #7949 presumably because the `res` name wasn't discoverable.
bors bot pushed a commit that referenced this pull request Sep 18, 2021
…`subalgebra.restrict_scalars` (#9251)

We use the name `restrict_scalars` everywhere else, so I kept that one instead of `res`.

`res` was here first, but the duplicate was added by #7949 presumably because the `res` name wasn't discoverable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated The PR author may merge after reviewing final suggestions. 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