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: Bool lemmas #183

Merged
merged 28 commits into from
Oct 27, 2023
Merged

feat: Bool lemmas #183

merged 28 commits into from
Oct 27, 2023

Conversation

fgdorais
Copy link
Collaborator

@fgdorais fgdorais commented Jul 10, 2023

@fgdorais fgdorais force-pushed the bool branch 7 times, most recently from 2f43344 to 2c3b74d Compare July 11, 2023 11:48
@fgdorais fgdorais marked this pull request as ready for review July 22, 2023 00:53
Std/Data/Bool.lean Outdated Show resolved Hide resolved
Std/Data/Bool.lean Outdated Show resolved Hide resolved
Std/Data/Bool.lean Outdated Show resolved Hide resolved
@fgdorais
Copy link
Collaborator Author

Updated to use new alias command from #200

@fgdorais fgdorais added the awaiting-author Waiting for PR author to address issues label Aug 21, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. label Aug 22, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. label Aug 22, 2023
@fgdorais fgdorais added awaiting-review This PR is ready for review; the author thinks it is ready to be merged. and removed awaiting-author Waiting for PR author to address issues labels Aug 22, 2023
Std/Data/Bool.lean Outdated Show resolved Hide resolved
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. label Sep 27, 2023
@fgdorais fgdorais mentioned this pull request Oct 22, 2023
1 task
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. label Oct 23, 2023
@joehendrix joehendrix added awaiting-author Waiting for PR author to address issues and removed awaiting-review This PR is ready for review; the author thinks it is ready to be merged. labels Oct 26, 2023
Std/Data/Bool.lean Show resolved Hide resolved
Std/Data/Bool.lean Outdated Show resolved Hide resolved
@joehendrix joehendrix merged commit 65544c3 into leanprover-community:main Oct 27, 2023
1 check passed
semorrison added a commit to leanprover-community/mathlib4 that referenced this pull request Oct 27, 2023
Comment on lines +32 to +35
instance : LE Bool := leOfOrd
instance : LT Bool := ltOfOrd
instance : Max Bool := maxOfLe
instance : Min Bool := minOfLe
Copy link
Member

@eric-wieser eric-wieser Oct 27, 2023

Choose a reason for hiding this comment

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

These change the defeq of the boolean order (used in mathlib) to be (IMO) worse. Previously, Mathlib had

instance linearOrder : LinearOrder Bool where
  le := fun a b ↦ a = false ∨ b = true
  max := or
  min := and

Was this a deliberate change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was a hope long ago that core would fixup Ord before this PR landed. Without that fixup, this is indeed a bad choice. My preferred versions are:

le x y := !x || y
lt x y := !x && y
max x y :=  x || y
min x y := x && y

These lead to the most efficient code in each case. I'm not sure if you really prefer your version for le? I will submit a small follow-up PR fixing this as soon as I have a chance.

@joehendrix joehendrix removed the awaiting-author Waiting for PR author to address issues label Oct 27, 2023
@digama0
Copy link
Member

digama0 commented Oct 29, 2023

I think this PR still needs changes, but I didn't have time to review it and things are a bit more complicated now. I told Scott to work around this for now for the mathlib bump, and I will try to make a follow up PR when I have the time.

bors bot pushed a commit to leanprover-community/mathlib4 that referenced this pull request Oct 29, 2023
Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
grunweg pushed a commit to leanprover-community/mathlib4 that referenced this pull request Nov 1, 2023
Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
fgdorais pushed a commit to leanprover-community/mathlib4 that referenced this pull request Nov 1, 2023
Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
bors bot pushed a commit to leanprover-community/mathlib4 that referenced this pull request Nov 2, 2023
We need `sup` and `max` to be defeq, and `max` is defined in Std. To achieve this, we define one in terms of the other.

Until #8105 was merged (which fixed up leanprover-community/batteries#183), they were not defeq:
```lean
example :
  (GeneralizedCoheytingAlgebra.toLattice : Lattice Bool).sup =
    DistribLattice.toLattice.sup :=
  rfl -- failed
```

This instance in this PR should have been written such that it couldn't silently cause a diamond in the first place, by reusing existing data rather than creating new data.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants