Skip to content

Conversation

YaelDillies
Copy link
Collaborator

@YaelDillies YaelDillies commented Oct 19, 2025

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


Open in Gitpod

@github-actions github-actions bot added the t-algebra Algebra (groups, rings, fields, etc) label Oct 19, 2025
@github-actions
Copy link

github-actions bot commented Oct 19, 2025

PR summary 0231ea944e

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference

Declarations diff

+ addEquivTuple
+ coe_addEquivTuple
+ coe_symm_addEquivTuple
+ instance : Add ℍ[R,c₁,c₂,c₃] := fast_instance% (equivProd c₁ c₂ c₃).add
+ instance : Neg ℍ[R,c₁,c₂,c₃] := fast_instance% (equivProd c₁ c₂ c₃).Neg
+ instance : SMul S ℍ[R,c₁,c₂,c₃] := fast_instance% (equivProd ..).smul _
+ instance : Sub ℍ[R,c₁,c₂,c₃] := fast_instance% (equivProd c₁ c₂ c₃).sub
+ instance : Zero ℍ[R,c₁,c₂,c₃] := fast_instance% (equivProd c₁ c₂ c₃).zero
+ instance [Monoid S] [MulAction S R] : MulAction S ℍ[R,c₁,c₂,c₃]
+ instance [Monoid S] [MulAction S R] : MulAction S ℍ[R]
+ instance [SMulCommClass S T R] : SMulCommClass S T ℍ[R,c₁,c₂,c₃] := (equivTuple ..).smulCommClass ..
+ 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 : Add ℍ[R,c₁,c₂,c₃]
- instance : Neg ℍ[R,c₁,c₂,c₃] := ⟨fun a => ⟨-a.1, -a.2, -a.3, -a.4⟩⟩
- instance : SMul S ℍ[R,c₁,c₂,c₃] where smul s a := ⟨s • a.1, s • a.2, s • a.3, s • a.4⟩
- instance : Sub ℍ[R,c₁,c₂,c₃]
- instance : Zero ℍ[R,c₁,c₂,c₃] := ⟨⟨0, 0, 0, 0⟩⟩
- instance [SMulCommClass S T R] : SMulCommClass S T ℍ[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).

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

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

@eric-wieser eric-wieser added the awaiting-author A reviewer has asked the author a question or requested changes. label Oct 19, 2025
@YaelDillies YaelDillies removed the awaiting-author A reviewer has asked the author a question or requested changes. label Oct 20, 2025
@[simps]
instance : Zero ℍ[R,c₁,c₂,c₃] := ⟨⟨0, 0, 0, 0⟩⟩
@[simps!]
instance : Zero ℍ[R,c₁,c₂,c₃] := fast_instance% (equivProd c₁ c₂ c₃).zero
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this produce a worse term?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@eric-wieser eric-wieser Oct 20, 2025

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@YaelDillies YaelDillies Oct 20, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-algebra Algebra (groups, rings, fields, etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants