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

feat(data/list/pi): new file #18417

Closed
wants to merge 9 commits into from
Closed

feat(data/list/pi): new file #18417

wants to merge 9 commits into from

Conversation

negiizhao
Copy link
Collaborator

@negiizhao negiizhao commented Feb 10, 2023

This file is an analog of data.multiset.pi. But this PR only adds some basic lemmas for #18315.

Move fin_enum.pi.* into this file. (nothing imports fin_enum!)


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. labels Feb 10, 2023
@negiizhao negiizhao removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Feb 10, 2023
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.

Can you add a lemma in multiset.pi that links list.pi.cons with multiset.pi.cons when m = coe l?

@eric-wieser eric-wieser added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Feb 10, 2023
@negiizhao
Copy link
Collaborator Author

multiset.pi has already been ported to mathlib4.

@negiizhao negiizhao added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Feb 11, 2023
/-- Given `α : ι → Sort*`, a list `l` and a term `i`, as well as a term `a : α i` and a
function `f` such that `f j : α j` for all `i'` in `m`, `pi.cons a f` is a function `g` such
that `g i'' : α i''` for all `i''` in `i :: l`. -/
def cons (a : α i) (f : Π j ∈ l, α j) : Π j ∈ (i :: l), α j :=
Copy link
Member

Choose a reason for hiding this comment

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

l should be explicit to match the multiset version

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In most cases it can be inferred automatically. Is there any disadvantage to making it implicit in multiset.pi.cons?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it requires making large changes to multiset.pi.cons which is already a ported file.

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.

I think we should modify multiset.pi anyway, and implement it as

quotient.rec_on m (λ l, @list.pi.cons _ _ _ _ l b) sorry

@eric-wieser
Copy link
Member

eric-wieser commented Feb 11, 2023

Or perhaps an easier option,

def list.pi.cons (l : list ι) {i : ι} (a : α i) (f : Π j ∈ l, α j) : Π j ∈ (i :: l), α j :=
multiset.pi.cons ↑l _ a f

@eric-wieser
Copy link
Member

Which leads to the question: why do you actually need this?

@negiizhao
Copy link
Collaborator Author

There is no multiset.pi.map_cons and multiset.pi.forall_rel_cons_ext in mathlib. We need to add some similar lemmas in any case, and I'm not sure it is a good idea to use multiset lemmas for lists. BTW it seems difficult to build APIs directly for multiset in subsequent PR (multiset.rec_on introduces annoying heq).

@eric-wieser
Copy link
Member

There is no multiset.pi.map_cons and multiset.pi.forall_rel_cons_ext in mathlib.

It sounds to me like you should add these lemmas first.

Comment on lines 29 to 30
@[simp] lemma cons_eta (f : Π j ∈ (i :: l), α j) :
cons (f i (mem_cons_self _ _)) (λ j hj, f j (mem_cons_of_mem _ hj)) = f :=
Copy link
Member

Choose a reason for hiding this comment

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

This lemma is multiset.pi.cons_ext

function `f` such that `f j : α j` for all `i'` in `m`, `pi.cons a f` is a function `g` such
that `g i'' : α i''` for all `i''` in `i :: l`. -/
def cons (a : α i) (f : Π j ∈ l, α j) : Π j ∈ (i :: l), α j :=
λ j hj, if h : j = i then h.symm.rec a else f j (hj.resolve_left h)
Copy link
Member

Choose a reason for hiding this comment

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

with #18429,

Suggested change
λ j hj, if h : j = i then h.symm.rec a else f j (hj.resolve_left h)
multiset.pi.cons ↑l _ a f

Comment on lines 35 to 36
(λ j hj, f ((cons h t) j hj)) = cons (f h) (λ j hj, f (t j hj)) :=
by { ext j hj, dsimp [cons], split_ifs with H, { cases H, refl }, { refl }, }
Copy link
Member

Choose a reason for hiding this comment

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

More lines but fewer tactics:

Suggested change
(λ j hj, f ((cons h t) j hj)) = cons (f h) (λ j hj, f (t j hj)) :=
by { ext j hj, dsimp [cons], split_ifs with H, { cases H, refl }, { refl }, }
(λ j hj, f ((cons h t) j hj)) = cons (f h) (λ j hj, f (t j hj)) :=
begin
ext j hj,
refine (apply_dite (@f _) _ _ _).trans (congr_arg2 _ _ rfl),
ext rfl,
refl,
end

Is there a reason you don't state it as

  f ((cons h t) j hj) = cons (f h) (λ j hj, f (t j hj)) j hj :=

which looks better for forward rewrites

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In #18315 I use it for a backward rewrite. Maybe I should swap its sides.

@eric-wieser
Copy link
Member

eric-wieser commented Feb 12, 2023

list.pi.cons already exists, it's fin_enum.pi.cons.

bors bot pushed a commit that referenced this pull request Feb 28, 2023
…18429)

Everything in this file about `multiset.pi.cons` generalizes to `Sort`.
The parts of the file which don't generalize now use `β` instead.

This is motivated by #18417, though I do not know if supporting `Sort` is actually important there.

Forward-ported as leanprover-community/mathlib4#2220
This discard the changes from #18429, as these are already present in the new version.
@Parcly-Taxel
Copy link
Collaborator

What is the status of this PR? It's apparently needed for leanprover-community/mathlib4#3971.

@eric-wieser
Copy link
Member

I think I'd like to split this PR into:

  • Move the existing definitions into a new file, with no modifications
  • Do the API cleanups

I think this will make it easier to forward-port.

@github-actions github-actions bot added the modifies-synchronized-file This PR touches a files that has already been ported to mathlib4, and may need a synchronization PR. label May 19, 2023
Copy link
Member

Choose a reason for hiding this comment

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

There's still too much going on in this file. I've created #19050 which captures the renames and the more obvious moves.

@eric-wieser eric-wieser added the not-too-late This PR was ready at the point mathlib3 was frozen: we will try to merge it and port it to mathlib4 label Jul 15, 2023
@semorrison
Copy link
Collaborator

Closing, replaced by leanprover-community/mathlib4#5549. Please reopen and ping me if this is incorrect!

@semorrison semorrison closed this Jul 27, 2023
@YaelDillies YaelDillies deleted the FR_list_pi branch November 18, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review The author would like community review of the PR modifies-synchronized-file This PR touches a files that has already been ported to mathlib4, and may need a synchronization PR. not-too-late This PR was ready at the point mathlib3 was frozen: we will try to merge it and port it to mathlib4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants