feat: NSMul/NPow type class#38036
feat: NSMul/NPow type class#38036JovanGerb wants to merge 15 commits intoleanprover-community:masterfrom
NSMul/NPow type class#38036Conversation
PR summary 4fceb4697eImport changes for modified filesNo significant changes to the import graph Import changes for all files
|
| Current number | Change | Type |
|---|---|---|
| 6219 | 1 | backward.isDefEq.respectTransparency |
| 795 | -2 | backward.privateInPublic |
| 412 | -1 | backward.privateInPublic.warn |
| 46 | -1 | dsimp +instances |
Current commit dcf7b585bf
Reference commit 4fceb4697e
You can run this locally as
./scripts/reporting/technical-debt-metrics.sh pr_summary
- The
relativevalue is the weighted sum of the differences with weight given by the inverse of the current value of the statistic. - The
absolutevalue is therelativevalue divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).
|
Could you elaborate on the benefit here? |
This can also be achieved with |
|
Sure, but then you lose the ability to write |
| @[to_additive] instance addCommMonoid : AddCommMonoid R[M] := | ||
| fast_instance% { (inferInstance : AddCommMonoid <| M →₀ R) with | ||
| nsmul n x := x.mapRange (n • ·) (smul_zero _) } | ||
| inferInstanceAs <| AddCommMonoid <| M →₀ R |
There was a problem hiding this comment.
What happened to fast_instance%?
There was a problem hiding this comment.
This is the great thing: inferInstanceAs will automatically fill in the nsmul field using the smul that was was declared above. (Recall that inferInstanceAs and fast_instance% do the same kind of instance normalization). Previously, we had to awkwardly declare the nsmul field in order to avoid diamond.
I think this should also answer your other question: yes we do need to infer NSMul instances from SMul instances, for example here.
|
!bench |
|
Benchmark results for 12eaeea against 0446312 are in. There are significant results. @JovanGerb
Large changes (1✅, 4🟥)
Medium changes (16✅, 10🟥) Too many entries to display here. View the full report on radar instead. Small changes (39✅, 69🟥) Too many entries to display here. View the full report on radar instead. |
|
!bench |
|
Benchmark results for 604ad3b against 0446312 are in. There are significant results. @JovanGerb
Large changes (1✅, 4🟥)
Medium changes (16✅, 11🟥) Too many entries to display here. View the full report on radar instead. Small changes (38✅, 68🟥) Too many entries to display here. View the full report on radar instead. |
|
It's curious how some files sped up while others slowed down. Maybe this change made some instances better - hence the speedup. But because of the extra extends, the structures are now a bit bigger, and hence unification takes longer (due to the exponentially slow unification thingy) |
|
!bench |
|
Benchmark results for dcf7b58 against 4fceb46 are in. There are significant results. @JovanGerb
Large changes (1✅, 4🟥)
Medium changes (14✅, 16🟥) Too many entries to display here. View the full report on radar instead. Small changes (42✅, 69🟥) Too many entries to display here. View the full report on radar instead. |
|
|
||
| Note that `MulOpposite.smul` is provided in an earlier file as it is needed to | ||
| provide the `AddMonoid.nsmul` and `AddCommGroup.zsmul` fields. | ||
| provide the `NSMul.nsmul` and `AddCommGroup.zsmul` fields. |
There was a problem hiding this comment.
Should this be
| provide the `NSMul.nsmul` and `AddCommGroup.zsmul` fields. | |
| provide the `NSMul.nsmul` and `ZSMul.zsmul` fields. |
|
|
||
| Note that `MulOpposite.smul` is provided in an earlier file as it is needed to | ||
| provide the `AddMonoid.nsmul` and `AddCommGroup.zsmul` fields. | ||
| provide the `NSMul.nsmul` and `AddCommGroup.zsmul` fields. |
There was a problem hiding this comment.
| provide the `NSMul.nsmul` and `AddCommGroup.zsmul` fields. | |
| provide the `NSMul.nsmul` and `ZSMul.zsmul` fields. |
This PR is adds
NSMul,NPow,ZSMulandZPowclasses for thensmul,npow,zsmul,zpowdata fields.This has a few advantages:
SMulinstance, then you don't need to manually writensmul := (· • ·)andzsmul := (· • ·). ForPow, the extra benefit is that inferring the instance is preferred over the default fieldnpowRecAuto. So this helps avoid accidental diamonds.SMulinstance on a type synonym, theninferInstanceAswill infer thensmulfield from theSMulinstance. This makes it easier to avoid diamonds on type synonyms likeMatrixandMonoidAlgebra.NSMulandSMulclasses do not agree.In the process of making this PR, I have identified two existing
NPowdiamonds:Mathlib.Algebra.Order.Positive.Field, there were two conflictingNPowinstances.Fin, there are two conflictingNPowinstances. I have overwritten the one in core lean with the one from mathlib that is more computationally efficient.TODO: the same for
QSMulandNNQSMul.