Skip to content
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

Two different skew-symmetry checks #2555

Closed
lgoettgens opened this issue Jul 13, 2023 · 1 comment · Fixed by #2566
Closed

Two different skew-symmetry checks #2555

lgoettgens opened this issue Jul 13, 2023 · 1 comment · Fixed by #2566
Labels
bug Something isn't working

Comments

@lgoettgens
Copy link
Member

lgoettgens commented Jul 13, 2023

Describe the bug
We have two different implementations for skew symmetry checks of a MatElem:

julia> methods(is_skew_symmetric)
# 1 method for generic function "is_skew_symmetric" from AbstractAlgebra:
 [1] is_skew_symmetric(M::MatElem)
     @ ~/path/AbstractAlgebra.jl/src/Matrix.jl:2403

julia> methods(is_skewsymmetric_matrix)
# 1 method for generic function "is_skewsymmetric_matrix" from Oscar:
 [1] is_skewsymmetric_matrix(B::MatElem{T}) where T<:RingElem
     @ ~/path/Oscar.jl/src/Groups/matrices/matrix_manipulation.jl:171

As far as I can tell, the implementations differ in the behavior in characteristic 2, and the latter imposes a restriction on the element type of the matrices.

It would be great to have only one function doing that and not two. Similar functions would be named is_skew_symmetric (without matrix), so I would like that name here.

Can somebody with knowledge about the use-cases say a few words about the characteristic 2 stuff, so that we can decide, which version to keep? In all cases, this should probably go into AbstractAlgebra.

@lgoettgens lgoettgens added the bug Something isn't working label Jul 13, 2023
@fingolfin
Copy link
Member

We are comparing

function is_skewsymmetric_matrix(B::MatElem{T}) where T <: RingElem
n = nrows(B)
n==ncols(B) || return false
for i in 1:n
B[i,i]==0 || return false
for j in i+1:n
B[i,j]==-B[j,i] || return false
end
end
return true
end

https://github.com/Nemocas/AbstractAlgebra.jl/blob/bfa185a7c074b20527009e039ad45be8ab1bdd7a/src/Matrix.jl#L2403-L2410

I would say the latter (in AbstractAlgebra) is what we want. The former (in Oscar) actually tests for an alternating form (resp. a representation matrix for such a form) -- we may also want such a test, but it should be under its own name. And indeed the restriction on the typ seems unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants