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(data): port finsupp.ne_locus/lex to dfinsupp #16777

Closed
wants to merge 15 commits into from

Conversation

alreadydone
Copy link
Collaborator

@alreadydone alreadydone commented Oct 3, 2022

  • Add new files dfinsupp/ne_locus and dfinsupp/lex mostly by copying the finsupp counterparts and making trivial adaptions. Use the dfinsupp lemmas/constructions to golf the finsupp counterpart, e.g. the linear_order on finsupp.lex.

  • Add lemmas lex_lt_of_lt(_of_preorder) for each of pi/finsupp/dfinsupp that shows the (<) relation on the product of a family of partial orders is a subrelation of the lexicographic (<), for any choice of well-founded relation (in the case of pi) or strict total order (in the case of (d)finsupp) on the set of coordinates. Useful to deduce well-foundedness of the product order from the well-foundedness of the lexicographic order in [Merged by Bors] - feat(data/(d)finsupp): well-foundedness of lexicographic and product orders #16772.


Open in Gitpod

@alreadydone alreadydone added awaiting-review The author would like community review of the PR t-order Order hierarchy labels Oct 3, 2022
Copy link
Collaborator

@adomani adomani left a comment

Choose a reason for hiding this comment

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

This looks very good!

When I was finishing the PR for finsupp I noticed that there was a possible generalisation to dfinsupp. However, I did not need it, nor had the time to invest in actually making it work.

I'm very happy that you are so energetic!

src/data/dfinsupp/lex.lean Outdated Show resolved Hide resolved
bors bot pushed a commit that referenced this pull request Oct 4, 2022
…order (#16740)

Included in #16777. Should be closed if #16777 is merged first.



Co-authored-by: Junyan Xu <junyanxu.math@gmail.com>
bors bot pushed a commit that referenced this pull request Oct 4, 2022
…order (#16740)

Included in #16777. Should be closed if #16777 is merged first.



Co-authored-by: Junyan Xu <junyanxu.math@gmail.com>
@mathlib-dependent-issues-bot mathlib-dependent-issues-bot added blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. and removed blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. labels Oct 4, 2022
Copy link
Collaborator

@YaelDillies YaelDillies left a comment

Choose a reason for hiding this comment

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

Your lemmas look very much like the dist ones in a normed group (dist_add_right, dist_neg_neg, dist_neg, dist_sub_left, ...) so I would align this API on that one (and you can see we're missing lemmas about ne_locus f (f + g) and similar) and leave a note to this effect.

Please also multiplicativize everything.

I understand this API is based off the finsupp.ne_locus one, but it is very recent and not imported further than data.finsupp.lex so I think it's fair changing.

Comment on lines 113 to 123
lemma lex.le_of_forall_le {a b : lex (Π₀ i, α i)} (h : ∀ i, of_lex a i ≤ of_lex b i) : a ≤ b :=
le_of_lt_or_eq $ or_iff_not_imp_right.2 $ λ hne, by classical; exact
⟨finset.min' _ (nonempty_ne_locus_iff.2 hne),
λ j hj, not_mem_ne_locus.1 (λ h, (finset.min'_le _ _ h).not_lt hj),
(h _).lt_of_ne (mem_ne_locus.1 $ finset.min'_mem _ _)⟩

lemma lex.le_of_of_lex_le {a b : lex (Π₀ i, α i)} (h : of_lex a ≤ of_lex b) : a ≤ b :=
lex.le_of_forall_le h

lemma to_lex_monotone : monotone (@to_lex (Π₀ i, α i)) :=
λ _ _, lex.le_of_forall_le
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of these four, the only one I would expect is to_lex_monotone and I would add its counterpart to_lex_strict_mono. The other ones have confusing names and if I were to discover the API for the first time I would wonder what their use is given that we already have to_lex_monotone and to_lex_strict_mono.

lex_lt_of_lt_of_preorder and lex_lt_of_lt are similarly unsettling because they mix bundled relations with unbundled ones.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Git blame tells me that all three were introduced by @urkud for pi.lex, with the first one in #9129 and the other two in #14389, and you've also touched the first one in #14164 before the other two were added. @adomani was probably just copying from pi.lex like what I do here. I'm happy to have the first two removed because all are defeq.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to confirm: I did simply copy a bunch of lemmas and fix as instructed by Lean!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lex_lt_of_lt_of_preorder and lex_lt_of_lt are similarly unsettling because they mix bundled relations with unbundled ones.

It's the only way to state them succinctly while still allowing complete generality on the relation on ι ; I'd like to use unbundled relation for the order on α i as well, but although a ≤ a' is just ∀ i, a i ≤ a' i (product order), a < a' is defined to be a ≤ a' ∧ a' ≰ a; we don't have a definition for this operation on relations, and I think writing (∀ i, r (a i) (a' i)) ∧ (∃ i, ¬ r (a' i) (a i)) is too long, so I rely on the typeclass system to generate the (<) for me, and that requires the use of preorder.

And really these are just 3 x 2 technical lemmas used to transfer well-foundedness of lex order to product order (in fact only the first of the dfinsupp lemmas is actually used, I believe).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @YaelDillies I've removed the 3x2 lemmas lex.le_of_forall_le/le_of_of_lex_le from pi/finsupp/dfinsupp and CI now passed. I also renamed the dfinsupp.ne_locus lemmas following your PR #17036. So the only thing not yet resolved is about lex_lt_of_lt(_of_preorder); do you have a better suggestion, or should I leave them as is?

src/data/dfinsupp/ne_locus.lean Outdated Show resolved Hide resolved
src/data/dfinsupp/ne_locus.lean Outdated Show resolved Hide resolved
src/data/dfinsupp/ne_locus.lean Outdated Show resolved Hide resolved
src/data/dfinsupp/ne_locus.lean Outdated Show resolved Hide resolved
src/data/dfinsupp/ne_locus.lean Show resolved Hide resolved
src/data/dfinsupp/ne_locus.lean Outdated Show resolved Hide resolved
@adomani
Copy link
Collaborator

adomani commented Oct 5, 2022

Just a heads-up that Anne delegated to me #16236 and I will merge is as soon as CI is happy with it. I'm mentioning this since the PR touches the finsupp.lex file, though only to edit a doc-string, I think.

@YaelDillies
Copy link
Collaborator

Oh nevermind we can't multiplicativize because we don't have finitely multiplicatively supported functions. I will open a PR matching the finsupp.ne_locus and dist APIs shortly.

Copy link
Collaborator Author

@alreadydone alreadydone left a comment

Choose a reason for hiding this comment

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

I will open a PR matching the finsupp.ne_locus and dist APIs shortly.

You other naming suggestions in ne_locus all look good, but don't you want to do those changes for finsupp and add the dfinsupp.ne_locus file together in your PR? If you do that part, I'll wait for it and only deal with this comment and not the others in this PR.

src/data/dfinsupp/ne_locus.lean Outdated Show resolved Hide resolved
@YaelDillies
Copy link
Collaborator

Here's the PR #17036. I don't care whether the current is merged first.

@jcommelin
Copy link
Member

Thanks 🎉

bors merge

@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 Oct 24, 2022
bors bot pushed a commit that referenced this pull request Oct 24, 2022
+ Add new files *dfinsupp/ne_locus* and *dfinsupp/lex* mostly by copying the finsupp counterparts and making trivial adaptions. Use the `dfinsupp` lemmas/constructions to golf the `finsupp` counterpart, e.g. the `linear_order` on `finsupp.lex`.

+ Add lemmas `lex_lt_of_lt(_of_preorder)` for each of pi/finsupp/dfinsupp that shows the (<) relation on the product of a family of partial orders is a subrelation of the lexicographic (<), for any choice of well-founded relation (in the case of pi) or strict total order (in the case of (d)finsupp) on the set of coordinates. Useful to deduce well-foundedness of the product order from the well-foundedness of the lexicographic order in #16772.



Co-authored-by: Junyan Xu <junyanxu.math@gmail.com>
@bors
Copy link

bors bot commented Oct 24, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(data): port finsupp.ne_locus/lex to dfinsupp [Merged by Bors] - feat(data): port finsupp.ne_locus/lex to dfinsupp Oct 24, 2022
@bors bors bot closed this Oct 24, 2022
@bors bors bot deleted the dfinsupp_port branch October 24, 2022 10:35
@eric-wieser eric-wieser added the hacktoberfest-accepted Without this label hacktoberfest is scared off by bors label Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Without this label hacktoberfest is scared off by bors ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) t-order Order hierarchy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants