[Merged by Bors] - refactor(Algebra/Quaternion): intermediate Module instance

[YaelDillies]

This Module instance allows me to not ungeneralise the NoZeroSMulDivisors R ℍ[R,c₁,c₂,c₃] in #30563.

---

[github-actions]

PR summary 6626d3a1ec

Import changes for modified files

No significant changes to the import graph

Import changes for all files

Files	Import difference

---

Declarations diff

+ addEquivProd
+ addEquivTuple
+ coe_addEquivProd
+ coe_addEquivTuple
+ coe_symm_addEquivProd
+ coe_symm_addEquivTuple
+ instance [AddCommGroup R] : AddCommGroup ℍ[R,c₁,c₂,c₃] := by
+ instance [Monoid S] [MulAction S R] : MulAction S ℍ[R,c₁,c₂,c₃]
+ instance [Monoid S] [MulAction S R] : MulAction S ℍ[R]
+ instance [Semiring S] [AddCommGroup R] [DistribMulAction S R] : DistribMulAction S ℍ[R,c₁,c₂,c₃]
+ instance [Semiring S] [AddCommGroup R] [Module S R] : Module S ℍ[R,c₁,c₂,c₃]
+ instance [Semiring S] [DistribMulAction S R] : DistribMulAction S ℍ[R]
+ instance [Semiring S] [Module S R] : Module S ℍ[R]
- instance [AddCommGroup R] : AddCommGroup ℍ[R,c₁,c₂,c₃]

You can run this locally as follows

## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/declarations_diff.sh long <optional_commit>

The doc-module for script/declarations_diff.sh contains some details about this script.

---

No changes to technical debt.

You can run this locally as

./scripts/technical-debt-metrics.sh pr_summary

• The relative value is the weighted sum of the differences with weight given by the inverse of the current value of the statistic.
• The absolute value is the relative value divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).

[eric-wieser]

Can you add a sentence in the PR description explaining what motivated these?

[eric-wieser]

Doesn't this produce a worse term?

[YaelDillies]

Sure, but the simp lemmas are just the same as before, so that should be no difference

[eric-wieser]

In terms of unification I think the previous approach was better.

[YaelDillies]

But unification with what? With other instances? They are all like that now

[eric-wieser]

Are you sure? Can you show me the effect of #print on the AddCommGroup instance before and after this line being changed?

[YaelDillies]

This is not possible. fast_instance% complains about the data fields not matching up to some transparency

[eric-wieser]

Ah, that's useful information! I was hoping it would use a higher level of transparency here.

In that case, I think it would be best to define things via the Function.Injective constructors. One argument for why this new zero instance is bad is that at runtime it's now a bunch of if statements to index a vector, rather than a hard-coded zero.

[YaelDillies]

No actually it is not using vectors. It used to be, because I was using equivTuple : _ \equiv (Fin 4 \r R), but I am now using equivProd : _ \equiv R \times R \times R \times R... unless you're claiming this too is bad at runtime?

[eric-wieser]

Ah, I missed that! Indeed equivProd is less bad, but still not as good as the direct constructor.

[YaelDillies]

Bad enough that I should use Function.Injective.foo? I was quite happy that I finally got to use the TransferInstance files!

[kckennylau]

!bench

[leanprover-radar]

Benchmark results for 8f39f20 against 67d9c26 are in! @kckennylau

Major changes (3)

• build//instructions changed by +7.8% (🟥).
• build/module/Mathlib.Algebra.Quaternion//instructions changed by +18.2G (🟥).
• open-mathlib//instructions changed by +5.9% (🟥).

Minor changes (2)

• build/module/Mathlib.Algebra.DualQuaternion//instructions changed by +6.9G (🟥).
• lint//instructions changed by +2.7% (🟥).

[leanprover-bot]

Here are the benchmark results for commit 8f39f20.
There were significant changes against commit 67d9c26:

  Benchmark                     Metric         Change
  ===================================================
- ~Mathlib.Algebra.Quaternion   instructions     9.5%

[github-actions]

File	Instructions	%
build	+32.918⬝10⁹	(+0.01%)
Mathlib.Algebra.Quaternion	+16.95⬝10⁹	(+9.50%)
Mathlib.Algebra.DualQuaternion	+5.383⬝10⁹	(+9.90%)
CI run Lakeprof report

[jcommelin]

@YaelDillies do you see where the performance regression comes from?

[YaelDillies]

@YaelDillies do you see where the performance regression comes from?

Probably because of Eric's concerns here: #30678 (comment). I've dropped the changes he was concerned about.

Note: CI now fails for reasons unrelated to the PR.

[jcommelin]

!bench

[leanprover-radar]

Benchmark results for 804d5a4 against 6626d3a are in! @jcommelin

No significant changes detected.

[jcommelin]

Thanks 🎉

bors merge

[mathlib-bors]

Pull request successfully merged into master.

Build succeeded:

• Build
• Lint style
• Post-Build Step