[Merged by Bors] - perf(NumberTheory.NumberField.Basic): make `RingOfIntegers` a Type. #12386

riccardobrasca · 2024-04-24T00:41:30Z

We make 𝓞 K a type rather then a Subalgebra to speed up instances search.

Co-authored-by: Anne Baanen <Vierkantor>
Co-authored-by: Yaël Dillies <YaelDillies>
Co-authored-by: Matthew Ballard <mattrobball>

riccardobrasca · 2024-04-24T01:26:39Z

!bench

leanprover-bot · 2024-04-24T01:54:14Z

Here are the benchmark results for commit 2ce7ff8.
There were significant changes against commit 90e480d:

  Benchmark                                                         Metric         Change
  =======================================================================================
+ ~Mathlib.NumberTheory.Cyclotomic.Rat                              instructions   -29.1%
+ ~Mathlib.NumberTheory.NumberField.Basic                           instructions   -33.6%
+ ~Mathlib.NumberTheory.NumberField.CanonicalEmbedding.Basic        instructions   -39.0%
+ ~Mathlib.NumberTheory.NumberField.CanonicalEmbedding.ConvexBody   instructions   -27.7%
+ ~Mathlib.NumberTheory.NumberField.ClassNumber                     instructions   -48.1%
+ ~Mathlib.NumberTheory.NumberField.Discriminant                    instructions   -26.0%
+ ~Mathlib.NumberTheory.NumberField.FractionalIdeal                 instructions   -61.1%
+ ~Mathlib.NumberTheory.NumberField.Norm                            instructions   -30.6%
+ ~Mathlib.NumberTheory.NumberField.Units                           instructions   -21.2%

Vierkantor

I will try to address the review comments I made in the next hours.

Mathlib/NumberTheory/Cyclotomic/Rat.lean

Mathlib/NumberTheory/NumberField/Basic.lean

Mathlib/NumberTheory/Cyclotomic/Rat.lean

Vierkantor · 2024-04-24T08:38:48Z

Mathlib/NumberTheory/NumberField/Basic.lean

+
+@[simp] lemma mk_zero : (⟨0, zero_mem _⟩ : 𝓞 K) = 0 :=
+  rfl
+-- TODO: these lemmas don't seem to fire?


We should figure out why simp dislikes these lemmas.

I tried for a bit and can't figure it out. If I write mk_add_mk _ then simp can use it (e.g. in restrict_addMonoidHom), but mk_add_mk is too hard.

If it works with _ and not without it, then there are some instance implicit arguments that cannot be found (or unified correctly) with typeclass synthesis. The underscore bypasses typeclass search for all arguments (if I remember correctly).

It's not because of the ⟨x, hx⟩ syntax?

If it works with _ and not without it, then there are some instance implicit arguments that cannot be found (or unified correctly) with typeclass synthesis. The underscore bypasses typeclass search for all arguments (if I remember correctly).

I forgot to give more details but they seem not to even be considered according to the set_option trace output. So it doesn't even appear to arrive at the instance synth step. But afaict the terms match perfectly syntactically: the discrimination tree should just find the simp lemmas.

riccardobrasca · 2024-04-25T02:07:53Z

!bench

leanprover-bot · 2024-04-25T02:33:51Z

Here are the benchmark results for commit 19ed662.
There were significant changes against commit d96c1e7:

  Benchmark                                                         Metric         Change
  =======================================================================================
+ ~Mathlib.NumberTheory.Cyclotomic.Rat                              instructions   -35.4%
+ ~Mathlib.NumberTheory.NumberField.Basic                           instructions   -32.0%
+ ~Mathlib.NumberTheory.NumberField.CanonicalEmbedding.Basic        instructions   -42.8%
+ ~Mathlib.NumberTheory.NumberField.CanonicalEmbedding.ConvexBody   instructions   -28.9%
+ ~Mathlib.NumberTheory.NumberField.ClassNumber                     instructions   -66.6%
+ ~Mathlib.NumberTheory.NumberField.Discriminant                    instructions   -26.1%
+ ~Mathlib.NumberTheory.NumberField.FractionalIdeal                 instructions   -71.9%
+ ~Mathlib.NumberTheory.NumberField.Norm                            instructions   -30.6%
+ ~Mathlib.NumberTheory.NumberField.Units                           instructions   -30.5%

kbuzzard

This is a very cool PR! It is kind of annoying that Lean 4's typeclass inference search is forcing us to do this, but on the other hand I don't really see any reason why we shouldn't do it anyway. IIRC we were already having problems with O_K as a subring being slow in Lean 3.

I have been thinking about the problem of how to set up this sort of problem on and off for years now, although as a non-specialist in type theory I have not really had good answers. In the Coq odd order proof they made all finite groups equal to subgroups of a large ambient group (so, in particular, terms). But this is not possible in ring theory because given two rings there might be no large ambient ring containing both as subrings (subrings must share a 1 with the ambient ring), so we are forced to reject this approach. IsScalarTower was our solution to making all rings equal to types, and for rings of integers of number fields we tried a different hybrid approach with the fields being types and the integer rings being terms, but the evidence we now have is that this is not as good a design pattern as just making everything a type anyway. I have had generations of students finding this part of the library painful to use; thanks a lot for making these changes!

Mathlib/NumberTheory/NumberField/Basic.lean

kbuzzard · 2024-04-27T19:02:32Z

Mathlib/NumberTheory/NumberField/CanonicalEmbedding/ConvexBody.lean

-      |(w₀.val.embedding a).re| < 1 ∧ |(w₀.val.embedding a).im| < (f w₀ : ℝ) ^ 2 := by
+    ∃ a : 𝓞 K, a ≠ 0 ∧ (∀ w : InfinitePlace K, w ≠ w₀ → w (algebraMap _ _ a) < f w) ∧
+      |(w₀.val.embedding (algebraMap _ _ a)).re| < 1 ∧
+      |(w₀.val.embedding (algebraMap _ _ a)).im| < (f w₀ : ℝ) ^ 2 := by


How about adding a coercion from O_K to K? I remember @Vierkantor once explaining to me why it was not possible to make algebraMap a coercion (something to do with head symbols, but I have forgotten the details, probably this means I didn't understand them properly). But is it possible here?

I didn't define the coercion because I thought it would require a lot more work to set up the API: that it preserves the ring operations etc. With algebraMap we get that for free, in exchange for a lot more writing.

The reason we can't say instance : CoeHead (𝓞 K) K := ⟨algebraMap _ _⟩ is that all the metaprogramming needs a @[coe] attribute on the expression to identify a coercion. Specifically on the head symbol. In this case, because algebraMap is a bundled function, the head symbol is different from algebraMap in this case. We'd need to define a synonym for this map that is not bundled and we can give that the @[coe] attribute.

Following what @erdOne suggests, the coercion be defined something as:

@[coe] abbrev val (x : 𝓞 K) : K := algebraMap _ _ x /-- This instance has to be `CoeHead` because we only want to apply it from `𝓞 K` to `K`. -/ instance : CoeHead (𝓞 K) K := ⟨val⟩

Although I can't say for sure if making val an abbrev would work, perhaps it has to be a def.

Mathlib/NumberTheory/NumberField/Norm.lean

kbuzzard · 2024-04-27T19:07:00Z

Mathlib/NumberTheory/NumberField/Units.lean

@@ -74,17 +75,21 @@ namespace NumberField.Units

 section coe

+-- TODO: figure out precise coercion
+instance : CoeHTC (𝓞 K)ˣ K :=
+  ⟨fun x => algebraMap _ K (Units.val x)⟩


So you want the units to coerce to K, but not the integers?

We have flt-regular as a stress test for this, where we use units a lot. If bumping mathlib is a lot of trouble I will be back to this.

Co-authored-by: Kevin Buzzard <k.buzzard@imperial.ac.uk>

Mathlib/NumberTheory/NumberField/Basic.lean

Vierkantor · 2024-04-29T16:47:40Z

I thought the same at the start of the refactor, now I'm slightly in favour of adding `.val` as a cast because it really is less cumbersome notation.

…

On 29 April 2024 11.50.36 CEST, Riccardo Brasca ***@***.***> wrote: @riccardobrasca commented on this pull request. > + +instance : CommRing (𝓞 K) := + inferInstanceAs (CommRing (integralClosure _ _)) +instance : IsDomain (𝓞 K) := + inferInstanceAs (IsDomain (integralClosure _ _)) +instance : CharZero (𝓞 K) := + inferInstanceAs (CharZero (integralClosure _ _)) +instance : Algebra (𝓞 K) K := + inferInstanceAs (Algebra (integralClosure _ _) _) +instance : NoZeroSMulDivisors (𝓞 K) K := + inferInstanceAs (NoZeroSMulDivisors (integralClosure _ _) _) +instance : Nontrivial (𝓞 K) := + inferInstanceAs (Nontrivial (integralClosure _ _)) + +variable {K} + Anyway do we really want this? It seems to me that this can just be confusing, while using `algebraMap` seems a rather natural way of doing this. -- Reply to this email directly or view it on GitHub: #12386 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

riccardobrasca · 2024-04-30T07:12:16Z

I thought the same at the start of the refactor, now I'm slightly in favour of adding .val as a cast because it really is less cumbersome notation.
…
On 29 April 2024 11.50.36 CEST, Riccardo Brasca @.> wrote: @riccardobrasca commented on this pull request. > + +instance : CommRing (𝓞 K) := + inferInstanceAs (CommRing (integralClosure _ _)) +instance : IsDomain (𝓞 K) := + inferInstanceAs (IsDomain (integralClosure _ _)) +instance : CharZero (𝓞 K) := + inferInstanceAs (CharZero (integralClosure _ _)) +instance : Algebra (𝓞 K) K := + inferInstanceAs (Algebra (integralClosure _ _) _) +instance : NoZeroSMulDivisors (𝓞 K) K := + inferInstanceAs (NoZeroSMulDivisors (integralClosure _ _) _) +instance : Nontrivial (𝓞 K) := + inferInstanceAs (Nontrivial (integralClosure _ _)) + +variable {K} + Anyway do we really want this? It seems to me that this can just be confusing, while using algebraMap seems a rather natural way of doing this. -- Reply to this email directly or view it on GitHub: #12386 (comment) You are receiving this because you were mentioned. Message ID: @.>

I am not really against it. Do you mean adding it as a coercion or just the notation?

Vierkantor · 2024-04-30T07:35:37Z

Do you mean adding it as a coercion or just the notation?

As a coercion. I don't think that simply defining .val is enough to improve the situation.

riccardobrasca · 2024-04-30T09:16:17Z

Do you mean adding it as a coercion or just the notation?

As a coercion. I don't think that simply defining .val is enough to improve the situation.

OK, I am having a look.

riccardobrasca · 2024-05-02T17:00:14Z

!bench

leanprover-bot · 2024-05-02T17:58:12Z

Here are the benchmark results for commit 7b28e54.
There were significant changes against commit 4e159e0:

  Benchmark                                                         Metric         Change
  =======================================================================================
+ ~Mathlib.NumberTheory.Cyclotomic.Rat                              instructions   -41.0%
+ ~Mathlib.NumberTheory.NumberField.Basic                           instructions   -48.0%
+ ~Mathlib.NumberTheory.NumberField.CanonicalEmbedding.Basic        instructions   -36.9%
+ ~Mathlib.NumberTheory.NumberField.CanonicalEmbedding.ConvexBody   instructions   -27.6%
+ ~Mathlib.NumberTheory.NumberField.ClassNumber                     instructions   -80.0%
+ ~Mathlib.NumberTheory.NumberField.Discriminant                    instructions   -28.8%
+ ~Mathlib.NumberTheory.NumberField.FractionalIdeal                 instructions   -76.3%
+ ~Mathlib.NumberTheory.NumberField.Norm                            instructions   -28.5%
+ ~Mathlib.NumberTheory.NumberField.Units.DirichletTheorem          instructions   -30.8%

mattrobball · 2024-05-03T13:15:38Z

Given it has already been blessed (and I barely touched it).

bors merge

…12386) We make `𝓞 K` a type rather then a `Subalgebra` to speed up instances search. Co-authored-by: Anne Baanen <[Vierkantor](https://github.com/Vierkantor)> Co-authored-by: Yaël Dillies <[YaelDillies](https://github.com/YaelDillies)> Co-authored-by: Matthew Ballard <[mattrobball](https://github.com/mattrobball)> Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com> Co-authored-by: Vierkantor <vierkantor@vierkantor.com> Co-authored-by: Yaël Dillies <yael.dillies@gmail.com> Co-authored-by: Matthew Ballard <matt@mrb.email> Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>

mathlib-bors · 2024-05-03T13:28:55Z

Build failed:

Build

Vierkantor · 2024-05-03T14:05:47Z

This looks more like a temporary build failure, so I'm going to retry:

bors merge

…12386) We make `𝓞 K` a type rather then a `Subalgebra` to speed up instances search. Co-authored-by: Anne Baanen <[Vierkantor](https://github.com/Vierkantor)> Co-authored-by: Yaël Dillies <[YaelDillies](https://github.com/YaelDillies)> Co-authored-by: Matthew Ballard <[mattrobball](https://github.com/mattrobball)> Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com> Co-authored-by: Matthew Ballard <matt@mrb.email> Co-authored-by: Vierkantor <vierkantor@vierkantor.com> Co-authored-by: Yaël Dillies <yael.dillies@gmail.com> Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>

mathlib-bors · 2024-05-03T14:57:36Z

Pull request successfully merged into master.

Build succeeded:

Mathlib/NumberTheory/NumberField/Basic.lean

Those have been forgotten in #12386.

…#11792) We add various result about the norm, relative to `ℤ`, of `ζ - 1`. These results already exist for the norm relative to `ℚ`. From the flt3 project at LFTCM2024. - [x] depends on: #12386 Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com> Co-authored-by: Vierkantor <vierkantor@vierkantor.com> Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com>

…12386) We make `𝓞 K` a type rather then a `Subalgebra` to speed up instances search. Co-authored-by: Anne Baanen <[Vierkantor](https://github.com/Vierkantor)> Co-authored-by: Yaël Dillies <[YaelDillies](https://github.com/YaelDillies)> Co-authored-by: Matthew Ballard <[mattrobball](https://github.com/mattrobball)> Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com> Co-authored-by: Matthew Ballard <matt@mrb.email> Co-authored-by: Vierkantor <vierkantor@vierkantor.com> Co-authored-by: Yaël Dillies <yael.dillies@gmail.com> Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>

Those have been forgotten in #12386.

…#11792) We add various result about the norm, relative to `ℤ`, of `ζ - 1`. These results already exist for the norm relative to `ℚ`. From the flt3 project at LFTCM2024. - [x] depends on: #12386 Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com> Co-authored-by: Vierkantor <vierkantor@vierkantor.com> Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com>

…12386) We make `𝓞 K` a type rather then a `Subalgebra` to speed up instances search. Co-authored-by: Anne Baanen <[Vierkantor](https://github.com/Vierkantor)> Co-authored-by: Yaël Dillies <[YaelDillies](https://github.com/YaelDillies)> Co-authored-by: Matthew Ballard <[mattrobball](https://github.com/mattrobball)> Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com> Co-authored-by: Matthew Ballard <matt@mrb.email> Co-authored-by: Vierkantor <vierkantor@vierkantor.com> Co-authored-by: Yaël Dillies <yael.dillies@gmail.com> Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>

Those have been forgotten in #12386.

…#11792) We add various result about the norm, relative to `ℤ`, of `ζ - 1`. These results already exist for the norm relative to `ℚ`. From the flt3 project at LFTCM2024. - [x] depends on: #12386 Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com> Co-authored-by: Vierkantor <vierkantor@vierkantor.com> Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com>

Turn `kernel : AddSubmonoid (α → MeasureTheory.Measure β)` into `Kernel : Type _`. This is similar to #12386 but here to gain access to dot notation rather than for performance reasons.

Vierkantor and others added 4 commits April 23, 2024 16:32

refactor: turn ringOfIntegers into a type RingOfIntegers

7029ff0

style

72c4630

fix

42828e0

fix

2ce7ff8

riccardobrasca changed the title ~~Ring of integer type~~ perf(NumberTheory.NumberField.Basic): make RingOfIntegers a Type. Apr 24, 2024

riccardobrasca added awaiting-review t-number-theory Number theory (also use t-algebra or t-analysis to specialize) t-algebra Algebra (groups, rings, fields, etc) labels Apr 24, 2024

riccardobrasca requested a review from kbuzzard April 24, 2024 02:45

Vierkantor reviewed Apr 24, 2024

View reviewed changes

Vierkantor and others added 9 commits April 24, 2024 11:26

Generalize CharZero instance

90fe9d7

With the generalization, we can drop this instance

f5164c8

Clean up proofs

30792d0

Use algebraMap instead of subtype casting

5ed31af

#noalign removed declaration

385deb6

Explain the change of definition in docstring

aa08e49

Improve simp slightly

fc2986d

Merge remote-tracking branch 'origin/master' into RingOfInteger_Type

e0c1318

Lean is not finding this in another file

19ed662

kbuzzard approved these changes Apr 27, 2024

View reviewed changes

Update Mathlib/NumberTheory/NumberField/Basic.lean

4b9dcf5

Co-authored-by: Kevin Buzzard <k.buzzard@imperial.ac.uk>

erdOne reviewed Apr 28, 2024

View reviewed changes

Mathlib/NumberTheory/NumberField/Basic.lean Show resolved Hide resolved

riccardobrasca added 3 commits May 2, 2024 18:32

Merge remote-tracking branch 'origin/master' into RingOfInteger_Type

92f6dcc

this is now useless

29506b6

not sure why Lean is not finding this

7b28e54

Merge branch 'master' into RingOfInteger_Type

4eee99a

mathlib-bors bot changed the title ~~perf(NumberTheory.NumberField.Basic): make RingOfIntegers a Type.~~ [Merged by Bors] - perf(NumberTheory.NumberField.Basic): make RingOfIntegers a Type. May 3, 2024

mathlib-bors bot closed this May 3, 2024

mathlib-bors bot deleted the RingOfInteger_Type branch May 3, 2024 14:57

riccardobrasca commented May 3, 2024

View reviewed changes

Mathlib/NumberTheory/NumberField/Basic.lean Show resolved Hide resolved

riccardobrasca mentioned this pull request May 6, 2024

[Merged by Bors] - feat: easy results about ring of integers. #12704

Closed

mathlib-bors bot pushed a commit that referenced this pull request May 7, 2024

feat: easy results about ring of integers. (#12704)

787f959

Those have been forgotten in #12386.

apnelson1 pushed a commit that referenced this pull request May 12, 2024

feat: easy results about ring of integers. (#12704)

e27775b

Those have been forgotten in #12386.

callesonne pushed a commit that referenced this pull request May 16, 2024

feat: easy results about ring of integers. (#12704)

0896ab4

Those have been forgotten in #12386.

YaelDillies mentioned this pull request Jul 12, 2024

feat: germs of smooth functions #10977

Open

YaelDillies mentioned this pull request Jul 22, 2024

[Merged by Bors] - refactor(Probability): Make kernels a type #15021

Closed

YaelDillies mentioned this pull request Jul 25, 2024

Tradeoff of defining types as subobjects leanprover-community/blog#86

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - perf(NumberTheory.NumberField.Basic): make `RingOfIntegers` a Type. #12386

[Merged by Bors] - perf(NumberTheory.NumberField.Basic): make `RingOfIntegers` a Type. #12386

riccardobrasca commented Apr 24, 2024 •

edited

Loading

riccardobrasca commented Apr 24, 2024

leanprover-bot commented Apr 24, 2024

Vierkantor left a comment

Vierkantor Apr 24, 2024

Vierkantor Apr 24, 2024

riccardobrasca Apr 25, 2024

mattrobball May 1, 2024

Ruben-VandeVelde May 1, 2024

Vierkantor May 2, 2024

riccardobrasca commented Apr 25, 2024

leanprover-bot commented Apr 25, 2024

kbuzzard left a comment •

edited

Loading

kbuzzard Apr 27, 2024

Vierkantor Apr 28, 2024

kbuzzard Apr 27, 2024

riccardobrasca Apr 28, 2024

Vierkantor commented Apr 29, 2024 via email

riccardobrasca commented Apr 30, 2024

Vierkantor commented Apr 30, 2024

riccardobrasca commented Apr 30, 2024

riccardobrasca commented May 2, 2024

leanprover-bot commented May 2, 2024

mattrobball commented May 3, 2024

mathlib-bors bot commented May 3, 2024

Vierkantor commented May 3, 2024

mathlib-bors bot commented May 3, 2024

[Merged by Bors] - perf(NumberTheory.NumberField.Basic): make RingOfIntegers a Type. #12386

[Merged by Bors] - perf(NumberTheory.NumberField.Basic): make RingOfIntegers a Type. #12386

Conversation

riccardobrasca commented Apr 24, 2024 • edited Loading

riccardobrasca commented Apr 24, 2024

leanprover-bot commented Apr 24, 2024

Vierkantor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riccardobrasca commented Apr 25, 2024

leanprover-bot commented Apr 25, 2024

kbuzzard left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vierkantor commented Apr 29, 2024 via email

riccardobrasca commented Apr 30, 2024

Vierkantor commented Apr 30, 2024

riccardobrasca commented Apr 30, 2024

riccardobrasca commented May 2, 2024

leanprover-bot commented May 2, 2024

mattrobball commented May 3, 2024

mathlib-bors bot commented May 3, 2024

Vierkantor commented May 3, 2024

mathlib-bors bot commented May 3, 2024

[Merged by Bors] - perf(NumberTheory.NumberField.Basic): make `RingOfIntegers` a Type. #12386

[Merged by Bors] - perf(NumberTheory.NumberField.Basic): make `RingOfIntegers` a Type. #12386

riccardobrasca commented Apr 24, 2024 •

edited

Loading

kbuzzard left a comment •

edited

Loading