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(field_theory/intermediate_field): introduce restrict_scalars which replaces lift2 #15191

Closed
wants to merge 6 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Jul 8, 2022

This brings the API in line with submodule and subalgebra by removing intermediate_field.lift2 and intermediate_field.has_lift2, and replacing them with intermediate_field.restrict_scalars. This definition is strictly more general than the previous intermediate_field.lift2 definition was. A few downstream lemma statements have been generalized in the same way.

The handful of API lemmas for restrict_scalars that this adds were already missing for lift2.

intermediate_field.lift2_alg_equiv has been removed since we didn't appear to have anything similar for subalgebra or submodule, but it's possible I missed it. At any rate, it's only needed in one proof, and we can just use show or refl instead.

Note that (↑x : intermediate_field F E) is not actually a shorter spelling than x.restrict_scalars F, especially when E is more than a single character.

Finally this renames lift1 to lift now that no ambiguity remains.


Open in Gitpod

@eric-wieser eric-wieser 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. labels Jul 8, 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 Jul 8, 2022
@@ -377,43 +376,45 @@ section tower

/-- Lift an intermediate_field of an intermediate_field -/
def lift1 {F : intermediate_field K L} (E : intermediate_field K F) : intermediate_field K L :=
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not reproduce the vector3 naming weirdness!

Suggested change
def lift1 {F : intermediate_field K L} (E : intermediate_field K F) : intermediate_field K L :=
def lift {F : intermediate_field K L} (E : intermediate_field K F) : intermediate_field K L :=

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing even uses this; shall I just remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

S.map val isn't horrible as a spelling, and we put up with it for submodules etc

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that we probably should get rid of it, but @kmill just introduced the subgraph analogues of it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure the subgraph-of-subgraph to subgraph map is a perfect analogue (I feel that combinatorial objects have somewhat different considerations from algebraic structures), but for what it's worth I've been making these sorts of shortcuts be @[reducible].

I'd vote don't remove it, and perhaps write add to the docstring to consider writing S.map val directly? That way someone trying to figure out how to do this lift can learn how to do it more easily.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds like enough of a resistance that removing it is out of scope of this PR.

I'll remove the 1 suffix for now, and propose a full removal in another PR.

@eric-wieser eric-wieser requested a review from kmill July 12, 2022 08:50
@@ -356,10 +359,10 @@ by { conv_rhs { rw ← adjoin_simple.algebra_map_gen F α },
rw is_integral_algebra_map_iff (algebra_map F⟮α⟯ E).injective,
apply_instance }

lemma adjoin_simple_adjoin_simple (β : E) : F⟮α⟯⟮β⟯ = F⟮α, β⟯ :=
lemma adjoin_simple_adjoin_simple (β : E) : F⟮α⟯⟮β⟯.restrict_scalars F = F⟮α, β⟯ :=
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wasn't sure where to leave this comment, so I'm choosing a random example.

Is F⟮α⟯⟮β⟯.restrict_scalars F now the simp normal form? I missed whether there was a simp lemma that rewrote ↑F⟮α⟯⟮β⟯ into a restrict_scalars expression.

Copy link
Member Author

@eric-wieser eric-wieser Jul 12, 2022

Choose a reason for hiding this comment

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

The coercion form no longer exists, for consistency with submodule.restrict_scalars and subalgebra.restrict_scalars which also have no coercion spelling.

@Vierkantor Vierkantor self-requested a review July 13, 2022 07:33
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.

I think this change is a good idea, especially since there is not much fallout in the dependencies. I'm not going to miss the coercion since usually you had to type-ascript it anyway.

@kmill, any further suggestions?

bors d=@kmill

src/field_theory/intermediate_field.lean Outdated Show resolved Hide resolved
[hFE : finite_dimensional F E]
[sp : p.is_splitting_field F E] (hp : p.separable)
(K : Type*) [field K] [algebra F K] [algebra K E]
[is_scalar_tower F K E] [finite_dimensional F K]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should follow from finite_dimensional F E + is_scalar_tower F K E (compare intermediate_field.finite_dimensional_left), but library_search isn't finding it at the moment...

Suggested change
[is_scalar_tower F K E] [finite_dimensional F K]
[is_scalar_tower F K E]

Copy link
Member Author

Choose a reason for hiding this comment

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

Inferring [finite_dimensional F K] from finite_dimensional F E + is_scalar_tower F K E isn't allowed because lean has no way to find E.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, the instance you're thinking of is found automatically when K is an intermediate_field

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but it can still be a theorem that we apply here. It turns out we have the other half: finite_dimensional.right, and basically the same should work for the left part.

Copy link
Member Author

Choose a reason for hiding this comment

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

The instance is needed in the theorem statement though in order to provide a fintype instance. This gets messy quite quickly; should we just take the fintype argument directly (since it carries data anyway?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's a proof:

theorem finite_dimensional.left (F K E : Type*) [field F] [field K] [field E]
  [algebra F K] [algebra K E] [algebra F E] [is_scalar_tower F K E]
  [finite_dimensional F E] :
  finite_dimensional F K :=
finite_dimensional.of_injective
  (is_scalar_tower.to_alg_hom F K E).to_linear_map
  (ring_hom.injective _)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The instance is needed in the theorem statement though in order to provide a fintype instance. This gets messy quite quickly; should we just take the fintype argument directly (since it carries data anyway?)

That's a good idea actually, there are in fact already non-defeq fintype instances on alg_hom.

Copy link
Member Author

Choose a reason for hiding this comment

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

Arguably none of this matters anyway; this is an auxiliary lemma

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've made the change anyway, it was straightforward.

@bors
Copy link

bors bot commented Jul 13, 2022

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

@leanprover-community-bot-assistant leanprover-community-bot-assistant added delegated The PR author may merge after reviewing final suggestions. and removed awaiting-review The author would like community review of the PR labels Jul 13, 2022
Vierkantor added a commit that referenced this pull request Jul 13, 2022
This result came up in the discussion of #15191, where I couldn't find it. (In the end we didn't up needing it.) I saw we already had finiteness of `L / K` (in fact, for any vector space instead of the field `L`) as `finite_dimensional.right`, so I made the `left` version too.

Also use this to provide an instance where `K` is an intermediate field.
@bors
Copy link

bors bot commented Jul 13, 2022

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

Co-authored-by: Kyle Miller <kmill31415@gmail.com>
@eric-wieser
Copy link
Member Author

bors merge

@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 Jul 13, 2022
bors bot pushed a commit that referenced this pull request Jul 13, 2022
…s` which replaces `lift2` (#15191)

This brings the API in line with `submodule` and `subalgebra` by removing `intermediate_field.lift2` and `intermediate_field.has_lift2`, and replacing them with `intermediate_field.restrict_scalars`. This definition is strictly more general than the previous `intermediate_field.lift2` definition was. A few downstream lemma statements have been generalized in the same way.

The handful of API lemmas for `restrict_scalars` that this adds were already missing for `lift2`.

`intermediate_field.lift2_alg_equiv` has been removed since we didn't appear to have anything similar for `subalgebra` or `submodule`, but it's possible I missed it. At any rate, it's only needed in one proof, and we can just use `show` or `refl` instead.

Note that `(↑x : intermediate_field F E)` is not actually a shorter spelling than `x.restrict_scalars F`, especially when `E` is more than a single character.

Finally this renames `lift1` to `lift` now that no ambiguity remains.
@bors
Copy link

bors bot commented Jul 13, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(field_theory/intermediate_field): introduce restrict_scalars which replaces lift2 [Merged by Bors] - refactor(field_theory/intermediate_field): introduce restrict_scalars which replaces lift2 Jul 13, 2022
@bors bors bot closed this Jul 13, 2022
@bors bors bot deleted the eric-wieser/remove-lift2 branch July 13, 2022 18:30
bors bot pushed a commit that referenced this pull request Jul 14, 2022
…5303)

This result came up in the discussion of #15191, where I couldn't find it. (In the end we didn't up needing it.) I saw we already had finiteness of `L / K` (in fact, for any vector space instead of the field `L`) as `finite_dimensional.right`, so I made the `left` version too.

Also use this to provide an instance where `K` is an intermediate field.





Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
bors bot pushed a commit that referenced this pull request Jul 14, 2022
…5303)

This result came up in the discussion of #15191, where I couldn't find it. (In the end we didn't up needing it.) I saw we already had finiteness of `L / K` (in fact, for any vector space instead of the field `L`) as `finite_dimensional.right`, so I made the `left` version too.

Also use this to provide an instance where `K` is an intermediate field.





Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
bors bot pushed a commit that referenced this pull request Jul 14, 2022
…5303)

This result came up in the discussion of #15191, where I couldn't find it. (In the end we didn't up needing it.) I saw we already had finiteness of `L / K` (in fact, for any vector space instead of the field `L`) as `finite_dimensional.right`, so I made the `left` version too.

Also use this to provide an instance where `K` is an intermediate field.





Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
bors bot pushed a commit that referenced this pull request Jul 15, 2022
…5303)

This result came up in the discussion of #15191, where I couldn't find it. (In the end we didn't up needing it.) I saw we already had finiteness of `L / K` (in fact, for any vector space instead of the field `L`) as `finite_dimensional.right`, so I made the `left` version too.

Also use this to provide an instance where `K` is an intermediate field.





Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
bors bot pushed a commit that referenced this pull request Jul 15, 2022
…5303)

This result came up in the discussion of #15191, where I couldn't find it. (In the end we didn't up needing it.) I saw we already had finiteness of `L / K` (in fact, for any vector space instead of the field `L`) as `finite_dimensional.right`, so I made the `left` version too.

Also use this to provide an instance where `K` is an intermediate field.





Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
bors bot pushed a commit that referenced this pull request Jul 15, 2022
…5303)

This result came up in the discussion of #15191, where I couldn't find it. (In the end we didn't up needing it.) I saw we already had finiteness of `L / K` (in fact, for any vector space instead of the field `L`) as `finite_dimensional.right`, so I made the `left` version too.

Also use this to provide an instance where `K` is an intermediate field.





Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
joelriou pushed a commit that referenced this pull request Jul 23, 2022
…s` which replaces `lift2` (#15191)

This brings the API in line with `submodule` and `subalgebra` by removing `intermediate_field.lift2` and `intermediate_field.has_lift2`, and replacing them with `intermediate_field.restrict_scalars`. This definition is strictly more general than the previous `intermediate_field.lift2` definition was. A few downstream lemma statements have been generalized in the same way.

The handful of API lemmas for `restrict_scalars` that this adds were already missing for `lift2`.

`intermediate_field.lift2_alg_equiv` has been removed since we didn't appear to have anything similar for `subalgebra` or `submodule`, but it's possible I missed it. At any rate, it's only needed in one proof, and we can just use `show` or `refl` instead.

Note that `(↑x : intermediate_field F E)` is not actually a shorter spelling than `x.restrict_scalars F`, especially when `E` is more than a single character.

Finally this renames `lift1` to `lift` now that no ambiguity remains.
joelriou pushed a commit that referenced this pull request Jul 23, 2022
…5303)

This result came up in the discussion of #15191, where I couldn't find it. (In the end we didn't up needing it.) I saw we already had finiteness of `L / K` (in fact, for any vector space instead of the field `L`) as `finite_dimensional.right`, so I made the `left` version too.

Also use this to provide an instance where `K` is an intermediate field.





Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
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

5 participants