-
Notifications
You must be signed in to change notification settings - Fork 839
refactor(Algebra/Quaternion): intermediate Module
instance
#30678
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
base: master
Are you sure you want to change the base?
refactor(Algebra/Quaternion): intermediate Module
instance
#30678
Conversation
PR summary 0231ea944eImport changes for modified filesNo significant changes to the import graph Import changes for all files
|
There was a problem hiding this 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?
@[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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
This
Module
instance allows me to not ungeneralise theNoZeroSMulDivisors R ℍ[R,c₁,c₂,c₃]
in #30563.