Experimental: Add Clifford algebras and Clifford orders #4370
Conversation
Provides Basic functionality for Clifford algebras over free quadratic lattices. Still a lot of work to do: Implement Clifford orders, handle pseudo-bases in the non-free case, etc.
This commit also contains minor changes to the CliffordAlgebras-files, mainly due to consistency with the CliffordOrders-files
Clifford orders are now fully functional. Some methods for Clifford algebras were renamed for consistency. Consequently the tests for Clifford algebras were modified as well.
divexact methods were implemented for Clifford orders. Also updated the existing one for Clifford algebras for consistency.
|
Why the ping? I was already happy, but @lgoettgens and @fingolfin had many comments and they should indicate whether they have been addressed. |
|
My comments are all resolved, but I am not familiar enough with the mathematics to approve this. |
|
@fingolfin could you have a quick look again regarding your comments? |
fingolfin
left a comment
fingolfin
left a comment
There was a problem hiding this comment.
There are a bunch of type stability issue. But since this is code for experimental, I think we could merge it.
One other thing I'd like to see added to this eventually are "ring interface conformance tests". I.e. calls to test_Ring_interface or better yet, test_Ring_interface_recursive. This may require adding test_elem methods.
| coeffs::Vector{S} where S<:NumFieldElem | ||
| even_coeffs::Vector{S} where S<:NumFieldElem | ||
| odd_coeffs::Vector{S} where S<:NumFieldElem |
There was a problem hiding this comment.
This declaration means access to these three vectors will be type unstable.
There was a problem hiding this comment.
This is fine to do (for reducing the number of method compilations) if you assert the concrete type every time you access these fields.
There was a problem hiding this comment.
In this commit I changed Vector{S} where S<:NumFieldElem to Vector{<:NumFieldElem}. Does this change anything?
There was a problem hiding this comment.
No, this has no real difference. What you could do here is to add a type assertion to each place you access these vectors (ideally only in a getter)
There was a problem hiding this comment.
As of this commit, these attributes are of type Any but the concrete type is now asserted at access time
| ################################################################################ | ||
|
|
||
| ### Algebra ### | ||
| mutable struct CliffordAlgebra{T,S} <: Hecke.AbstractAssociativeAlgebra{T} |
There was a problem hiding this comment.
What are T and S? I would recommend to e.g. add a comment indicating what kind of types to expect here. People other than the original author who need to modify the code will probably grateful (based on experience that includes "the author in 1 year" ;-)
There was a problem hiding this comment.
I added some clarifying comments in this commit.
| dim::Int | ||
| basis_of_centroid::Any | ||
| disq::T | ||
| basis_of_center::Any |
There was a problem hiding this comment.
Lots of underspecified members (base_ring, space, basis_of_centroid, basis_of_center) meaning that code accessing them directly will be type unstable. You could compensate for that by using accessor function that have type assertions with the correct rings. It seems you already do so for base_ring and space but not for the others?
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
| two elements. The first one is the multiplicative identity of $C$. The square of | ||
| the second basis element, if present, equals `quadratic_discriminant(C)`. | ||
| """ | ||
| function basis_of_centroid(C::CliffordAlgebra)::Vector{elem_type(C)} |
There was a problem hiding this comment.
| function basis_of_centroid(C::CliffordAlgebra)::Vector{elem_type(C)} | |
| function basis_of_centroid(C::CliffordAlgebra) |
This lowers to a type conversion. To get a type assertion, instead put this to each line with a return, eg here return C.basis_of_centroid::Vector{elem_type(C)}. (at all places you just modified
There was a problem hiding this comment.
I did not know that, thanks. It should work now, due to this commit
| """ | ||
| function even_coefficients(x::CliffordAlgebraElem) | ||
| if isdefined(x, :even_coeffs) | ||
| return x.even_coeffs |
There was a problem hiding this comment.
This eg needs a type assertion as well (and 2 times below, and the other functions here)
There was a problem hiding this comment.
Really? I thought the type declarations of the data structure would be sufficient in this case.
There was a problem hiding this comment.
Yes they are. I didn't notice that you changed that up there
|
|
||
| Return the vector of coefficient ideals of the canonical pseudo-basis of $C$. | ||
| """ | ||
| coefficient_ideals(C::CliffordOrder) = C.coefficient_ideals::Vector{typeof(fractional_ideal(base_ring(C), one(base_ring(C))))} |
There was a problem hiding this comment.
I believe there is fractional_ideal_type to get this type at compile time (ie without typeof). Maybe you need to write Hecke.fractional_ideal_type (not sure, maybe @thofma knows)
There was a problem hiding this comment.
Hecke.fractional_ideal_type(base_ring(C)) should do the trick.
There was a problem hiding this comment.
Even better to use Hecke.fractional_ideal_type(base_ring_type(C)) to make it compile time computable
This commit adds many generic methods for elements of Clifford orders that are not automatically available from AbstractAssociativeAlgebra. Examples are: minimal_polynomial, characteristic_polynomial, inv, etc.
|
I think all comments are resovled now. And after all, this only adds experimental code, so it can just we revised later. Let's get this in. And thanks again to @SirToby25 for putting work into this! |
lgoettgens
changed the title
lgoettgens
added
the
release notes: use title
fingolfin
changed the title
thofma
changed the title
5 participants
This is the first approach of implementing Clifford algebras and Clifford orders for quadratic spaces and lattices. There will be some future development, but it already allows for the standard computations one might want to perform within these structures.
Resolves #2312