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/finset/basic): Add decidable_nonempty for finsets. #15170

Closed
wants to merge 9 commits into from

Conversation

linesthatinterlace
Copy link
Collaborator

@linesthatinterlace linesthatinterlace commented Jul 7, 2022

Also remove some redundant decidable instances in multiset and list.


Note: as we have the nonempty predicate, we can define a decidable instance for it without worrying about diamond issues. This is always possible, even if there is no decidable equality on the underlying type.

@linesthatinterlace linesthatinterlace 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 Jul 7, 2022
@linesthatinterlace
Copy link
Collaborator Author

Not sure if this failure is related to the commit or not.

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.

The failure is caused by this application of tauto:

have y = x ∨ y = g x hx, by simp [hy] at hy₁; tauto,

Replacing it with sorry makes the error go away.

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.

This seems like a better proof to me:

instance _root_.multiset.decidable_exists_mem {P : α → Prop} [decidable_pred P] (m : multiset α) :
  decidable (∃ a ∈ m, P a) :=
quotient.rec_on_subsingleton m list.decidable_exists_mem

instance decidable_exists_mem {P : α → Prop} [decidable_pred P] (s : finset α) :
  decidable (∃ a ∈ s, P a) :=
s.val.decidable_exists_mem

instance decidable_nonempty {s : finset α} : decidable (s.nonempty) :=
decidable_of_iff (∃ a ∈ s, true) $ by simp_rw [exists_prop, and_true, finset.nonempty]

where obviously the multiset instance belongs in another file.

@linesthatinterlace
Copy link
Collaborator Author

I also added decidable_forall_mem for both of these as list has it.

I could also add nonempty lemmas for list. Also, the forall and exists lemmas for list are in a very odd place - so I could move them somewhere more appropriate.

What do you think?

@eric-wieser
Copy link
Member

Since you were able to put the new lemmas in the right place without any import trouble, I don't think moving the list lemmas is worth the effort.

This looks good now - can you update the PR description?

@linesthatinterlace linesthatinterlace changed the title Add decidable instance for finset.nonempty. Add decidable instances for finset and multiset. Jul 8, 2022
@linesthatinterlace
Copy link
Collaborator Author

Description updated.

@eric-wieser
Copy link
Member

PR title also doesn't match the commit guidelines (it's missing the prefix)

@linesthatinterlace
Copy link
Collaborator Author

Oh bother. Should it be feat(data/finset/basic) (despite the multiset lemmas?)

@linesthatinterlace linesthatinterlace changed the title Add decidable instances for finset and multiset. feat(data/finset/basic): Add decidable instances for finset and multiset. Jul 8, 2022
@github-actions github-actions bot removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Jul 8, 2022
src/data/finset/basic.lean Outdated Show resolved Hide resolved
src/data/multiset/basic.lean Outdated Show resolved Hide resolved
@linesthatinterlace
Copy link
Collaborator Author

Shall I remove these duplicate instances and just push decidable_nonempty?

@eric-wieser
Copy link
Member

@linesthatinterlace: Please do; removing the existing list instance would be good too.

@linesthatinterlace linesthatinterlace changed the title feat(data/finset/basic): Add decidable instances for finset and multiset. feat(data/finset/basic): Add decidable_nonempty for finsets. Jul 11, 2022
@linesthatinterlace
Copy link
Collaborator Author

Done; let's see if removing those causes any build issues...

@linesthatinterlace
Copy link
Collaborator Author

Yes: counter to what @YaelDillies said, the list versions do appear to be used. Do we have the dependent list versions?

@linesthatinterlace
Copy link
Collaborator Author

We have list.all₂.decidable_pred, which appears to be... another version of one of the non-dependent versions? But I think it uses the existing decidable_forall_mem instances also? This is a mess.

@YaelDillies
Copy link
Collaborator

The dependent multiset version uses the non-dependent one, and this looks like it causes a direct non-defeq diamond (but someone should check). Maybe the non-dependent one should not be an instance, then?

@linesthatinterlace
Copy link
Collaborator Author

linesthatinterlace commented Jul 11, 2022

No, I think they might use decidable_ball and decidable_bexists. These are core instances of the non-dependable ones for list. It seems like decidable_forall_mem and decidable_exists_mem just duplicate them.

@linesthatinterlace
Copy link
Collaborator Author

linesthatinterlace commented Jul 11, 2022

Current state:
list

  • decidable_ball and decidable_bexists. Non-dependent list instances that are in core, not mathlib.
  • decidable_forall_mem and decidable_exists_mem. Non-dependent list instances that are in mathlib. They duplicate the above.
  • all₂.decidable_pred. Equivalent to decidable_ball but specifically using the all₂ definition (not defeq).

multiset

  • Have decidable_forall_multiset, decidable_dforall_multiset, decidable_eq_pi_multiset, decidable_exists_multiset, and decidable_dexists_multiset, where decidable_forall_multiset and decidable_exists_multiset are essentially the non-dependent versions. One of these currently uses decidable_exists_mem but it doesn't have to.

finset

  • Have decidable_dforall_finset, decidable_eq_pi_finset, and decidable_dexists_finset. Do not have the non-dependent versions at all as far as I can see.

I am, if I am honest, unsure what we "should" have for each of these, and what they should be called. Do we want decidable_dforall_list, decidable_eq_pi_list, and decidable_dexists_list, say? That might mean we don't need the non-dependent versions for multiset either.

@linesthatinterlace
Copy link
Collaborator Author

Note that my last comment should really wait for another PR. This one is now ready to go, I think - the total changes just remove the list instances we don't need and adds decidable_nonempty. @eric-wieser, are you now happy?

Copy link
Collaborator

@kmill kmill left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks. Please remove the parentheses, then feel free to merge.

bors d+

src/data/finset/basic.lean Outdated Show resolved Hide resolved
@bors
Copy link

bors bot commented Jul 13, 2022

✌️ linesthatinterlace can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@leanprover-community-bot-assistant leanprover-community-bot-assistant added delegated The PR author may merge after reviewing final suggestions. and removed awaiting-review The author would like community review of the PR labels Jul 13, 2022
Co-authored-by: Kyle Miller <kmill31415@gmail.com>
@linesthatinterlace
Copy link
Collaborator Author

bors r+

bors bot pushed a commit that referenced this pull request Jul 13, 2022
Also remove some redundant decidable instances in multiset and list.



Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: Wrenna Robson <34025592+linesthatinterlace@users.noreply.github.com>
@bors
Copy link

bors bot commented Jul 13, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(data/finset/basic): Add decidable_nonempty for finsets. [Merged by Bors] - feat(data/finset/basic): Add decidable_nonempty for finsets. Jul 13, 2022
@bors bors bot closed this Jul 13, 2022
@bors bors bot deleted the decidable_nonempty branch July 13, 2022 23:45
joelriou pushed a commit that referenced this pull request Jul 23, 2022
Also remove some redundant decidable instances in multiset and list.



Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: Wrenna Robson <34025592+linesthatinterlace@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated The PR author may merge after reviewing final suggestions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants