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

refactor: make charP_to_charZero an instance #6548

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ChrisHughes24
Copy link
Member


This should be safe now that instance cycles are allowed in Lean4

Open in Gitpod

@kbuzzard
Copy link
Member

I've seen those build errors before, when changing unrelated files. Typeclass inference spends a long time doing a stupid thing (it's ring1 failing to prove F[X][X] has char 0), and random changes can push it over the edge. Either increase the numbers or disable the instance which is causing all the problems (it's StrictOrderedSemiring.to_charZero, you can attribute [-instance] it)

@ChrisHughes24
Copy link
Member Author

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 5629904.
There were significant changes against commit 8712fab:

  Benchmark                             Metric           Change
  =============================================================
- build                                 native linking    11.1%
- ~Mathlib.NumberTheory.SumTwoSquares   instructions       5.8%

@ocfnash
Copy link
Contributor

ocfnash commented Aug 19, 2023

Empirically typeclass cycles are still bad, at least in my experience. I recently tried to promote two theorems to instances and ran into non-trivial problems (to say nothing of performance hits). I mentioned this here:
https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/type.20class.20inference.20looping/near/385635549

IMHO, the situation is better than Lean 3 but I think typeclass inference still has a lot of room for improvement in Lean 4 (which is actually pretty exciting, as I believe we'll see it happen).

So unfortunately I think we should not make this change.

Copy link
Contributor

@ocfnash ocfnash left a comment

Choose a reason for hiding this comment

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

I'd like to have this but I think Mathlib / Lean is not yet ready.

@@ -75,6 +75,9 @@ local macro "map_simp" : tactic =>

universe u v w

-- Disabled locally for performance reasons
attribute [-instance] StrictOrderedSemiring.to_charZero
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to the disappointing benchmark results I think this is a pretty big red flag.

@ocfnash ocfnash added awaiting-author A reviewer has asked the author a question or requested changes t-algebra Algebra (groups, rings, fields etc) and removed awaiting-review labels Aug 19, 2023
@bors bors bot changed the base branch from master to ScottCarnahan/BinomialRing2 September 17, 2023 03:26
@semorrison semorrison changed the base branch from ScottCarnahan/BinomialRing2 to master September 17, 2023 12:13
@ocfnash
Copy link
Contributor

ocfnash commented Oct 3, 2023

I just came across this PR again (tidying my notifications) and it occurs to me that the right fix would be to change the definition of the CharP class to the mathematically more sensible:

class CharP [AddMonoidWithOne R] (p : ℕ) : Prop where
  cast_eq_iff' : ∀ x y : ℕ, (x : R) = (y : R) ↔ (x : ZMod p) = (y : ZMod p)

and then delete CharZero since it is then equivalent (even without additive inverses) to CharP R 0.

For bonus points one could even rename CharP to Char!

@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 Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-author A reviewer has asked the author a question or requested changes merge-conflict The PR has a merge conflict with master, and needs manual merging. t-algebra Algebra (groups, rings, fields etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants