Inconsistency in the interface between fields and vector spaces

[kwankyu]

There is an inconsistency in the interface between fields and vector spaces.

sage: F.<a>=GF(9)
sage: V=F.vector_space()
sage: V(a)
(0, 1)
sage: G.<b>=GF(3)
sage: W=G.vector_space()
sage: W(b)
...
TypeError: can't initialize vector from nonzero non-list

The cause is

sage: a._vector_()
(0, 1)
sage: b._vector_()
...
AttributeError: 'sage.rings.finite_rings.integer_mod.IntegerMod_int' object has no attribute '_vector_'

Adding a _vector_ method to the GF(p) elements will be a simple fix.

Along the way, we also clean up _vector_ and _matrix_ methods in the class FinitePolyExtElement.

Component: finite rings

Author: Vincent Delecroix, Kwankyu Lee

Branch/Commit: c1748cf

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/21723

[videlec]

comment:1

Note that the following also works

sage: K = GF(9)
sage: V = VectorSpace(K, 1)
sage: V(K.an_element())
(0)

Hence I guess that a default _vector_ at the category level would actually make sense.

[videlec]

Commit: d2b48ad

[videlec]

Branch: u/vdelecroix/21723

[videlec]

Author: Vincent Delecroi

[videlec]

comment:2

three lines fix... let us see how much it breaks Sage.

---

New commits:

d2b48ad

21723: make a default `_vector_` for ring elements

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

520b20f

21723: more appropriate doctest

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from d2b48ad to 520b20f

[kwankyu]

comment:4

Two comments:

(1) For elements of many other parents, the code is k.vector_space()(lst) where k is the parent. Hence for further consistency, we may consider add vector_space parent method as well. But then Rings category is not the right one...

(2) The docstring does not have an initial one-line summary.

[kwankyu]

comment:5

I fear that using the category framework right might be complicated in reality, contrary to the theory.

An alternative (old-fashioned :-) path is to add element_prime_modn.py, in which we subclass IntegerMod for modulo p case and add _vector_ method therein. I think this fits in the current organization of finite fields machinery. We may also need to add vector_space method to FiniteField_prime_modn class..

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

52a2830

21723: add a vector_space methods to fields

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 520b20f to 52a2830

[videlec]

comment:7

The _vector_ method for elements has no link to the vector_space method for parents.

I fear that using the category framework right might be complicated in reality, contrary to the theory.

This is not an argument. Moreover using category allows to have these methods available for any rings or fields.

[videlec]

Changed author from Vincent Delecroi to Vincent Delecroix

[kwankyu]

comment:9

Replying to @videlec:

This is not an argument. Moreover using category allows to have these methods available for any rings or fields.

_vector_ is anyhow related with some vector space, the precise meaning of which would depend on the kind of rings. For GF(9), it is a 2-dimensional vector space over GF(3) while for GF(3), it is a vector space over itself. You implemented _vector_ assuming that the parent ring is seen as a vector space over itself, disregarding the kind of of the ring. As a principle, methods in a category should be meaningful and useful for all parents in the category. So I think the Rings category is not the right category to put your _vector_ implementation. On the other hand, it seems tricky to find the right category in the current Sage. [I expected finite fields belong to a finite-dimensional algebras category. But not true.]

[videlec]

comment:10

The part that was missing in the current implementation is the basic construction ring -> one dimensional vector space over this ring. This construction makes sense for any ring and hence makes sense in Sage category framework.

You are right that for some specific ring, there might be other natural vector spaces. In this case it is possible to add more specialized implementations. And this is exactly what is being done for finite fields.

[videlec]

comment:11

And something more general that could be done is to see GF(2^4) as a vector space over GF(2^2). But the current interface does not allow it.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

bc95cac

21723: simplify `_vector_` and `_matrix_` of finite field elements

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 52a2830 to bc95cac

[kwankyu]

comment:13

(1) You added vector_space method to the fields category. How can this be useful at all? You can always construct the vector spaces directly by QQ*n.

(2) About _matrix_ method: Its docstring says "Return the matrix of right multiplication by the element on the power basis 1, x, x^2, \ldots, x^{d-1} for the field extension. Thus the \emph{rows} of this matrix give the images of each of the x^i." So we have

sage: b=a^10
sage: v=b._vector_()
sage: m=a._matrix_()
sage: w=(a*b)._vector_()
sage: m*v == w
True

Shouldn't this be called the "matrix of left multiplication by the element a"? And the columns of this matrix give the images of x^i?

(3) The reverse argument for _matrix_ method corresponds to reverseed _vector_. Therefore I expect m*v == w still holds with reversed versions. But

sage: v=b._vector_(reverse=True)
sage: m=a._matrix_(reverse=True)
sage: w=(a*b)._vector_(reverse=True)
sage: m*v == w
False
sage: m*v
(1, 1, 1, 1)
sage: w
(1, 1, 1, 0)
sage: 

I think this is a bug. Why does transposing the matrix gives what we want? The correct way would be reversing all columns and all rows.

By the way, you changed the docstring for reverse argument. But the original one makes more sense.

[kwankyu]

comment:14

Prime finite fields already have vector_space method.

sage: F=GF(3)
sage: V=F.vector_space()
sage: V([F.gen()])
(1)

So the bug dealt with the present ticket is just in the prime finite field. I still think that touching the Rings category is not the right way.

[videlec]

comment:15

Replying to @kwankyu:

So the bug dealt with the present ticket is just in the prime finite field. I still think that touching the Rings category is not the right way.

Why?

[kwankyu]

comment:16

Replying to @videlec:

Replying to @kwankyu:

So the bug dealt with the present ticket is just in the prime finite field. I still think that touching the Rings category is not the right way.

Why?

Before:

sage: K.<a>=QQ.extension(x^2-2)
sage: K.vector_space()

(Vector space of dimension 2 over Rational Field, Isomorphism map:
   From: Vector space of dimension 2 over Rational Field
   To:   Number Field in a with defining polynomial x^2 - 2, Isomorphism map:
   From: Number Field in a with defining polynomial x^2 - 2
   To:   Vector space of dimension 2 over Rational Field)
sage: a.vector()
(0, 1)
sage: vector(a)
(0, 1)

After your patch:

sage: a.vector()
(0, 1)
sage: vector(a)
(a)

So .vector() and ._vector_() should be context-sensitive.

[tscrim]

comment:17

The ._vector_() in that case should be an alias of .vector(). What the vector(a) is doing is calling list(a) and then interpreting that as a vector. I'm +1 for putting something generic in Rings since it helps realize that any ring is a 1-dim free module over itself. Although we should put some specification in the docstring; something along the lines of "Return self as a vector in some natural module."

[videlec]

comment:18

But as Kwankyu mentioned it is not what we want for field extensions. We should found a consistent way of doing the identification ring = one-dimensional free module but without overlap with the existing field extension business.

[tscrim]

comment:19

It's a similar problem to what we have with gens being ambiguous, but for gens, we could get out of it by being more explicit about what type of generators. For _vector_, we don't have the option of specializing the name of the method, but we can be very careful with the specs. In particular, by saying "some natural module," we give the option for subclasses to specify what that natural module is, which could change. So in the generic case, it is the 1-dim module over itself, but for field extensions, the natural module is the one over the base field.

[kwankyu]

Alternative light-footed patch

[kwankyu]

comment:20

Attachment: trac21723_klee.patch.gz

Adding _vector_ to a category level would incur much work to make existing parents conform to the new organization. It may be possible and in the long run is a way to go. But in this particular case, I think that the gain is small compared to the needed efforts.

I uploaded an alternative simple way to fix the bug. This just does a small thing to squeeze the bug. If you agree, then you may merge it with your other changes.

[videlec]

comment:21

I agree that your patch might fix the symptom. As discussed with Travis, the real problem is that _vector_ and vector do not have a fixed semantic. I am in favor of including your patch in this ticket alone and try to fix the _vector_/vector issue in some other tickets. Travis?

[videlec]

comment:22

21737

[kwankyu]

comment:23

I like the changes you made to _vector_ and _matrix_ methods in element_base.pyx modulo my comments above. If you are ok, I may take it to fix the bug in _matrix_ method that I mentioned above. Anyway, do you agree with my comments on the _matrix_ method?

[kwankyu]

Attachment: trac21723matrix.patch.gz

Fix a bug in reversed _matrix_

[tscrim]

comment:24

I'm good with moving the general problem to another ticket and fixing the symptom here. Kwankyu, could you upload your fix as a new branch?

[kwankyu]

Changed commit from bc95cac to none

[kwankyu]

Changed branch from u/vdelecroix/21723 to public/21723

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

4b17ccd	Add `_vector_` method to IntegerMod elements
aa1ff7b	Fix a bug in reversed _matrix_

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Commit: aa1ff7b

[kwankyu]

Changed author from Vincent Delecroix to Vincent Delecroix, Kwankyu Lee

[kwankyu]

Description changed:

--- 
+++ 
@@ -21,3 +21,5 @@

Adding a _vector_ method to the GF(p) elements will be a simple fix. + +Along the way, we also clean up _vector_ and _matrix_ methods in the class FinitePolyExtElement.

[tscrim]

comment:29

One little thing: Example:: -> EXAMPLES:: and then I'm happy setting a positive review.

[tscrim]

Reviewer: Travis Scrimshaw

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

0b0935c

Refined docstrings

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from aa1ff7b to 0b0935c

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

c1748cf

Corrected minor typos in docstrings

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 0b0935c to c1748cf

[kwankyu]

comment:32

Travis?

[tscrim]

comment:33

Thanks.

[vbraun]

Changed branch from public/21723 to c1748cf