No type piracy: remove matrix*vector methods for Nemo types

[fingolfin]

No description provided.

[tthsqe12]

Status? AbstractAlgebra has matrix vector products where the vector is a julia vector. Internally, this delegates to a mul! function which will be overridden in Nemo soon for efficiency with common types.

[fingolfin]

Let's try to resolve this one way or another: either by deleting the methods which multiple a Generic.FreeModuleElem by a MatElm; or by moving this to AbstractAlgebra.

Deleting it as this PR does leads to additional test failures that I could fix. But the core issue remains: people have a matrix group, say GL(3,GF(5)) and would like to "act on vectors". How do they get appropriate vectors?

The naive approach is to use V = vector_space(GF(5),3) (in GAP one would do V := GF(5)^3) and try to act on that. This is what the tests and some code examples do right now. Can we support this sensibly?

If not, then we need some other way to get a suitable "vector space" on which the group acts. Perhaps via a function like natural_module(G::MatrixGroup). Here I'd expect the result to be a "vector space" / F-module (for some field F), and not a G-module (though ultimately I guess a G-module would also be fine, as long as one can work with vector in a sensible way.

How to proceed? Hoping for some input from @fieker

[fingolfin]

OK I remember now why we can't just move this to AA (at least not easily): because general matrices are not square, and so M*v in general has a different parent than v, but no such parent is part of the input... This is a bit different for matrix group elements, which of course always are square.

So this would suggest to "resolve" (?) the issue here by removing the code which pretends to multiply free module elements by a MatElm, and instead restrict those methods to MatrixGroupElem .

[fingolfin]

Hrm, though of course code like this also breaks if we remove those FreeModuleElem * MatElem methods.

   V = vector_space(F,6)
   L = [1,4,6,2,3,5]
   P = permutation_matrix(F,L)
   @testset for i in 1:6
      @test V[i]*P == V[L[i]]
   end

[fingolfin]

Regarding support for v*M or M*v where v is a FreeModuleElem and M is a matrix, there could be a * method showing a helpful error pointing about a documentation section that explain why this doesn't work, and indicates working alternatives (here: either turn M into a map, and how to do so; or turn v into a matrix (with a single row/column).

Regarding conjugate_transpose: this should use a FrobeniusCtx from Hecke. Also care must be taken about relative field extension: the current code may be incorrect for them (so add test cases).

[fingolfin]

@fieker unfortunately FrobeniusCtx currently is only implemented for fqPolyRepFieldElem. So I guess before using that the first step needs to be to generalize it -- perhaps with a generic inefficient fallback (so one can always use it) and then we can gradually add optimized versions for the various finite field types -- or at least for our "default fields"

[fingolfin]

Unfortunately we use this in one book example in the group theory chapter:

julia> R2 = free_module(K, 2) # the "euclidean" plane over K
Vector space of dimension 2 over QQBar

julia> A = R2([0,1])
(Root 0 of x, Root 1.00000 of x - 1)

julia> pts = [A*mat_rot^i for i in 0:4];

However if we replace A*mat_rot by A*G_mat[1] it is fine because we retained that method -- we can retain it as (a) it is not type piracy, and (b) matrix group elements are guaranteed to be square).

So we either need to modify the book example or keep this method... Guess we can decide it when we decide how to proceed with the book.

[fingolfin]

Since the book is now being revised I can adjust the book example to match, I guess. But I suggest that is done first in the book, and then in all repositories which have stuff derived from it. And only once that all is through do we merge this here.

Repositories that contain code extract from book examples

• Oscar.jl of course
• https://github.com/oscar-system/book-notebooks (do we still care about it?)
• https://github.com/oscar-system/book.oscar-system.org
• anything else?

[fingolfin]

This really needs to be settled one way or another this Wednesday.

[fingolfin]

I discussed this with @fieker this morning and it turns out we don't even need the linke pts = [A*mat_rot^i for i in 0:4]; in the book example anymore! So I have now taken it out of the book, and that means this obstacle is gone.

Just need to wait for the book examples to be synced into this repository

[fingolfin]

This reflects a change in the book, i.e. we removed the definition of A and pts and tweaked that of R2 for the book.

[lgoettgens]

I am happy with the changes here. However, I don't like changing the book tests in this PR. Instead, I would suggest to wait until #4205 has been forwardported (or whatever the word for backporting in the other direction is) onto master and then rebase this here on top of that.

[lgoettgens]

@fingolfin now that #4221 is merged, can you rebase this onto master so that this PR no longer touches the book tests?

[lgoettgens]

Looks good to merge from my POV. Any objection @ThomasBreuer ?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.58%. Comparing base (9ef11df) to head (a011fee).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #760   +/-   ##
=======================================
  Coverage   84.58%   84.58%           
=======================================
  Files         631      631           
  Lines       84999    84994    -5     
=======================================
- Hits        71899    71895    -4     
+ Misses      13100    13099    -1     

Files with missing lines	Coverage Δ
src/Groups/matrices/forms.jl	93.49% <100.00%> (+0.03%)	⬆️
src/Groups/matrices/matrix_manipulation.jl	98.59% <ø> (+1.18%)	⬆️

[ThomasBreuer]

No objection