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(data/finsupp/antidiagonal): Make antidiagonal a finset #7595

Closed
wants to merge 6 commits into from

Conversation

BoltonBailey
Copy link
Collaborator

@BoltonBailey BoltonBailey commented May 12, 2021

Pursuant to discussion here

Refactoring so that finsupp.antidiagonal and multiset.antidiagonal are finsets.

Still TO DO: multiset.antidiagonal


Open in Gitpod

@BoltonBailey BoltonBailey added the WIP Work in progress label May 12, 2021
@digama0
Copy link
Member

digama0 commented May 12, 2021

Also mentioned on zulip, but I disagree with the refactor of multiset.antidiagonal. That function is written exactly the way it is intended to be. The antidiagonal function that I think Bolton wants could however be extracted into a typeclass on ordered monoids:

class has_antidiagonal (α : Type*) [ordered_add_comm_monoid α] :=
(antidiagonal : α → finset (α × α))
(mem_antidiagonal {{a b c : α}} : (a, b) ∈ antidiagonal c ↔ a + b = c)

This has instances for nat, multiset, finset, and α →₀ β where β has an antidiagonal function. Maybe it needs a different name to distinguish it from the existing function though.

@BoltonBailey
Copy link
Collaborator Author

I like @digama0 's suggestion of a has_antidiagonal class, but I am working on another project and don't have time to implement it right now. Can I ask that this PR be reviewed now and an issue be opened so that the has_antidiagonal class is still on the mathlib radar?

@BoltonBailey BoltonBailey added awaiting-review The author would like community review of the PR and removed WIP Work in progress labels May 16, 2021
@fpvandoorn
Copy link
Member

Can I ask that this PR be reviewed now

I think Mario's suggestion is to not remove the current version of finsupp.antidiagonal, but add a new class / definition that has the behavior you want. Your PR removes the current finsupp.antidiagonal.
I also think that the current definition of finsupp.antidiagonal (before this PR) is a natural one to have, but feel free (as Mario suggested) to rename it to finsupp.antidiagonal' and add finsupp.antidiagonal for its support (or define the above class has_antidiagonal).

@BoltonBailey
Copy link
Collaborator Author

I also think that the current definition of finsupp.antidiagonal (before this PR) is a natural one to have, but feel free (as Mario suggested) to rename it to finsupp.antidiagonal' and add finsupp.antidiagonal for its support (or define the above class has_antidiagonal).

Yes, thanks for reminding me of this. I have now restored the original definition as finsupp.antidiagonal'.

@BoltonBailey
Copy link
Collaborator Author

Created ticket for has_antidiagonal here

Copy link
Member

@fpvandoorn fpvandoorn left a comment

Choose a reason for hiding this comment

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

LGTM

src/data/finsupp/antidiagonal.lean Outdated Show resolved Hide resolved
Co-authored-by: Floris van Doorn <fpvdoorn@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
f.antidiagonal'.support

@[simp] lemma mem_antidiagonal {f : α →₀ ℕ} {p : (α →₀ ℕ) × (α →₀ ℕ)} :
p ∈ antidiagonal f ↔ p.1 + p.2 = f :=
begin
rcases p with ⟨p₁, p₂⟩,
simp [antidiagonal, ← and.assoc, ← finsupp.to_multiset.apply_eq_iff_eq]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
simp [antidiagonal, ← and.assoc, ← finsupp.to_multiset.apply_eq_iff_eq]
simp [antidiagonal, antidiagonal', ← and.assoc, ← finsupp.to_multiset.apply_eq_iff_eq]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change needs to be made in two places, I have committed it, we'll see if it fixes it.

@fpvandoorn
Copy link
Member

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 May 17, 2021
bors bot pushed a commit that referenced this pull request May 17, 2021
Pursuant to discussion [here](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/antidiagonals.20having.20multiplicity) 

Refactoring so that `finsupp.antidiagonal` and `multiset.antidiagonal` are finsets.

~~Still TO DO: `multiset.antidiagonal`~~
@bors
Copy link

bors bot commented May 17, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(data/finsupp/antidiagonal): Make antidiagonal a finset [Merged by Bors] - refactor(data/finsupp/antidiagonal): Make antidiagonal a finset May 17, 2021
@bors bors bot closed this May 17, 2021
@bors bors bot deleted the antidiagonal_refactor branch May 17, 2021 16:52
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

4 participants