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(ring_theory/noetherian): fg_pi #10845

Closed
wants to merge 6 commits into from

Conversation

eric-wieser
Copy link
Member


Open in Gitpod

@eric-wieser eric-wieser added the awaiting-review The author would like community review of the PR label Dec 16, 2021
@riccardobrasca
Copy link
Member

riccardobrasca commented Dec 16, 2021

Something like

theorem fg_pi {ι : Type*} [fintype ι] {φ : ι → Type*} [Π (i : ι), add_comm_monoid (φ i)]
  [Π (i : ι), module R (φ i)] {p : Π (i : ι), submodule R (φ i)} (hsb : ∀ i, (p i).fg) :
  (submodule.pi set.univ p).fg

should be true, and more general if I understand submodule.pi, but Lean is not happy about this statement. OPS, I was in the wrong file, the statement is OK.

@@ -184,6 +184,16 @@ fg_def.2 ⟨linear_map.inl R M P '' tb ∪ linear_map.inr R M P '' tc,
(htb.1.image _).union (htc.1.image _),
by rw [linear_map.span_inl_union_inr, htb.2, htc.2]⟩

theorem fg_pi {ι : Type*} [fintype ι] {p : ι → submodule R M} (hsb : ∀ i, (p i).fg) :
Copy link
Member

Choose a reason for hiding this comment

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

theorem fg_pi {ι : Type*} [fintype ι] {φ : ι → Type*} [Π (i : ι), add_comm_monoid (φ i)]
  [Π (i : ι), module R (φ i)] {p : Π (i : ι), submodule R (φ i)} (hsb : ∀ i, (p i).fg) :
  (submodule.pi set.univ p).fg

is a more general statement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh you're quite right, that was careless of me

@riccardobrasca
Copy link
Member

Can you change linear_algebra/free_module/strong_rank_condition.lean as in #10757? (This was my original motivation for this instance, then I will close #10757). Johan suggested that

instance module.finite.pi {ι : Type*} [fintype ι] (M : Type*) [add_comm_monoid M] [module R M] [module.finite R M] : module.finite R (ι → M)

needs to be manually added to help Lean.

@eric-wieser
Copy link
Member Author

eric-wieser commented Dec 16, 2021

That doesn't work I'm afraid, to make lean happy I need one of these special cases:

instance pi' {ι : Type*} {M : Type*} [fintype ι] [non_unital_non_assoc_semiring M]
  [module R M] [finite R M] : finite R (ι → M) :=
by apply_instance
instance pi' {ι : Type*} {M : Type*} [fintype ι] [add_comm_group M]
  [module R M] [finite R M] : finite R (ι → M) :=
by apply_instance

Weakening non_unital_non_assoc_semiring to add_comm_monoid causes the problem.

@riccardobrasca
Copy link
Member

The first one seems a little weird to me (since a module in general will only have one operation), but the second one is perfectly fine. I think we have finite modules over semiring just because it was for free, but nobody really cares.

Comment on lines +46 to +48
-- Lean is unable to find this instance without help, either via this `letI`, or via a duplicate
-- instance with unecessarily strong typeclasses on `R` and `M`.
letI : module.finite R (fin n.succ → R) := module.finite.pi,
Copy link
Member Author

Choose a reason for hiding this comment

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

This seems like the same problem I was experiencing in #8221, where typeclass search doesn't try hard enough to find the right semiring R and add_comm_monoid arguments that match the module argument it found. Rather than adding a pile of global instances just to be safe, my hope is lean4 will solve this.

@@ -43,6 +43,10 @@ begin
{ rwa strong_rank_condition_iff_succ R },
intros n f, by_contradiction hf,

-- Lean is unable to find this instance without help, either via this `letI`, or via a duplicate
-- instance with unecessarily strong typeclasses on `R` and `M`.
letI : module.finite R (fin n.succ → R) := module.finite.pi,
Copy link
Member

Choose a reason for hiding this comment

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

I will let you decide, but I have the impression that it is better to add a global instance that make Lean automatically find this particular one (and maybe others somewhere else).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, if you think it will be solved in Lean 4 then OK :)

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 hope if lean4 doesn't solve it, we can push for it to be changed so that it does :). I'm not opposed to adding the global instance if we keep running into this, but would prefer to wait for more data and keep things conservative in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, this looks like something that could be solved in Lean 4 by solving the metavariables in a smarter order.

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, thanks!

bors r+

@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 Dec 17, 2021
bors bot pushed a commit that referenced this pull request Dec 17, 2021
@bors
Copy link

bors bot commented Dec 17, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(ring_theory/noetherian): fg_pi [Merged by Bors] - feat(ring_theory/noetherian): fg_pi Dec 17, 2021
@bors bors bot closed this Dec 17, 2021
@bors bors bot deleted the eric-wieser/fg_pi branch December 17, 2021 16:44
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