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): Make Finsupp.filter computable #8979

Closed
wants to merge 9 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Dec 11, 2023

This doesn't have any significant downstream fallout, and removes some subsingleton elimination from one or two proofs.

This enables some trivial computations on factorizations, eg finding the odd prime factors:

/-- info: fun₀ | 3 => 2 | 5 => 1 -/
#guard_msgs in
#eval (Nat.factorization 720).filter Odd

Zulip thread


Open in Gitpod

This doesn't have any significant downstream effect.
@eric-wieser eric-wieser added awaiting-review The author would like community review of the PR awaiting-CI labels Dec 11, 2023
@eric-wieser eric-wieser marked this pull request as ready for review December 11, 2023 15:29
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Jan 18, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Jan 22, 2024
Mathlib/Data/Finsupp/Basic.lean Outdated Show resolved Hide resolved
Mathlib/Data/Finsupp/Basic.lean Outdated Show resolved Hide resolved
Mathlib/Algebra/MonoidAlgebra/Division.lean Outdated Show resolved Hide resolved
@urkud
Copy link
Member

urkud commented Jan 29, 2024

AFAIR, many definitions about Finsupp used to be computable, then we made them noncomputable. I think that making random subset of them computable again is a bad design choice: we should either decide that we make as many as possible computable, or leave all of them noncomputable and add a meta program that, e.g., lifts Finsupp operations to the corresponding computable DFinsupp operations.

Note: I'm definitely not an expert in computability; I think that it's better to discuss this on Zulip.

@urkud urkud added RFC Request for comment t-algebra Algebra (groups, rings, fields etc) labels Jan 29, 2024
@eric-wieser eric-wieser force-pushed the eric-wieser/finsupp-filter-decidable branch from 200288b to 71eb07c Compare February 11, 2024 15:32
support :=
haveI := Classical.decPred p
f.support.filter fun a => p a
def filter (p : α → Prop) [DecidablePred p] (f : α →₀ M) : α →₀ M where
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand that if somebody wants to use Finsupp.filter, either their predicate is decidable (possibly by adding that to the instances of the theorem), and then everything's OK, otherwise one invokes Classical.decPred before using it, as you do below in coeff_weightedHomogeneousComponent .
It would probably be interesting that there be a general page on mathlib's documentation about this, hoping that everybody will care and agree.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately I don't think we have any good mechanism at the moment to host such a general documentation page; library notes are not yet shown in doc-gen...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would the mathlib overview host a section on decidability?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately I don't think we have any good mechanism at the moment to host such a general documentation page; library notes are not yet shown in doc-gen...

At least temporarily the github wiki feature could host things like this?

@@ -966,9 +959,11 @@ theorem prod_div_prod_filter [CommGroup G] (g : α → M → G) :

end Zero

theorem filter_pos_add_filter_neg [AddZeroClass M] (f : α →₀ M) (p : α → Prop) :
theorem filter_pos_add_filter_neg [AddZeroClass M] (f : α →₀ M) (p : α → Prop) [DecidablePred p] :
Copy link
Member

Choose a reason for hiding this comment

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

I think that we should also assume DecidablePred (¬p ·) so that this lemma applies in case if the second filter uses a classical instance.

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise LGTM. Thanks!
bors d+

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 current statement matches DFinsupp,filter_pos_add_filter_neg, so I'd prefer to declare this out of scope. I think we should try doing this to everything that uses ¬p at once, and see if anything breaks: in a separate PR.

@mathlib-bors
Copy link

mathlib-bors bot commented Feb 13, 2024

✌️ 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.

@eric-wieser eric-wieser force-pushed the eric-wieser/finsupp-filter-decidable branch from 3ec7517 to 71eb07c Compare February 23, 2024 00:17
@eric-wieser
Copy link
Member Author

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review The author would like community review of the PR labels Feb 23, 2024
mathlib-bors bot pushed a commit that referenced this pull request Feb 23, 2024
This doesn't have any significant downstream fallout, and removes some subsingleton elimination from one or two proofs.

This enables some trivial computations on factorizations, eg finding the odd prime factors:
```lean
/-- info: fun₀ | 3 => 2 | 5 => 1 -/
#guard_msgs in
#eval (Nat.factorization 720).filter Odd
```

[Zulip thread](https://leanprover.zulipchat.com/#narrow/stream/144837-PR-reviews/topic/.238979.20making.20Finsupp.2Efilter.20computable/near/420898226)
@mathlib-bors
Copy link

mathlib-bors bot commented Feb 23, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title refactor(Data/Finsupp): Make Finsupp.filter computable [Merged by Bors] - refactor(Data/Finsupp): Make Finsupp.filter computable Feb 23, 2024
@mathlib-bors mathlib-bors bot closed this Feb 23, 2024
@mathlib-bors mathlib-bors bot deleted the eric-wieser/finsupp-filter-decidable branch February 23, 2024 01:41
thorimur pushed a commit that referenced this pull request Feb 24, 2024
This doesn't have any significant downstream fallout, and removes some subsingleton elimination from one or two proofs.

This enables some trivial computations on factorizations, eg finding the odd prime factors:
```lean
/-- info: fun₀ | 3 => 2 | 5 => 1 -/
#guard_msgs in
#eval (Nat.factorization 720).filter Odd
```

[Zulip thread](https://leanprover.zulipchat.com/#narrow/stream/144837-PR-reviews/topic/.238979.20making.20Finsupp.2Efilter.20computable/near/420898226)
thorimur pushed a commit that referenced this pull request Feb 26, 2024
This doesn't have any significant downstream fallout, and removes some subsingleton elimination from one or two proofs.

This enables some trivial computations on factorizations, eg finding the odd prime factors:
```lean
/-- info: fun₀ | 3 => 2 | 5 => 1 -/
#guard_msgs in
#eval (Nat.factorization 720).filter Odd
```

[Zulip thread](https://leanprover.zulipchat.com/#narrow/stream/144837-PR-reviews/topic/.238979.20making.20Finsupp.2Efilter.20computable/near/420898226)
riccardobrasca pushed a commit that referenced this pull request Mar 1, 2024
This doesn't have any significant downstream fallout, and removes some subsingleton elimination from one or two proofs.

This enables some trivial computations on factorizations, eg finding the odd prime factors:
```lean
/-- info: fun₀ | 3 => 2 | 5 => 1 -/
#guard_msgs in
#eval (Nat.factorization 720).filter Odd
```

[Zulip thread](https://leanprover.zulipchat.com/#narrow/stream/144837-PR-reviews/topic/.238979.20making.20Finsupp.2Efilter.20computable/near/420898226)
dagurtomas pushed a commit that referenced this pull request Mar 22, 2024
This doesn't have any significant downstream fallout, and removes some subsingleton elimination from one or two proofs.

This enables some trivial computations on factorizations, eg finding the odd prime factors:
```lean
/-- info: fun₀ | 3 => 2 | 5 => 1 -/
#guard_msgs in
#eval (Nat.factorization 720).filter Odd
```

[Zulip thread](https://leanprover.zulipchat.com/#narrow/stream/144837-PR-reviews/topic/.238979.20making.20Finsupp.2Efilter.20computable/near/420898226)
Louddy pushed a commit that referenced this pull request Apr 15, 2024
This doesn't have any significant downstream fallout, and removes some subsingleton elimination from one or two proofs.

This enables some trivial computations on factorizations, eg finding the odd prime factors:
```lean
/-- info: fun₀ | 3 => 2 | 5 => 1 -/
#guard_msgs in
#eval (Nat.factorization 720).filter Odd
```

[Zulip thread](https://leanprover.zulipchat.com/#narrow/stream/144837-PR-reviews/topic/.238979.20making.20Finsupp.2Efilter.20computable/near/420898226)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has been sent to bors. RFC Request for comment t-algebra Algebra (groups, rings, fields etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants