search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.37k stars 466 forks source link

Finite field lattices via Conway polynomials #8335

Closed roed314 closed 11 years ago

roed314 commented 14 years ago

Implements coercion within lattices of finite fields lying above the same prime when implemented with Conway polynomials.

sage: k = GF(9, conway=True, prefix='z')
sage: l = GF(27, conway=True, prefix='z')
sage: x = k.gen() + l.gen(); x
z6^5 + 2*z6^4 + 2*z6^3 + z6^2 + 2*z6 + 1
sage: x.parent()
Finite Field in z6 of size 3^6

When using the conway and prefix parameters, one does not need to specify an explicit variable name; if no variable name is given, it is constructed from the prefix and the degree (as in the above code snippet).

In the future, the functionality of this ticket will be incorporated into that for algebraic closures of finite fields. It will then be possible to construct compatible systems of finite fields outside the range of the Conway polynomial database using the pseudo-Conway polynomials from #14958: polynomials that satisfy all of the algebraic constraints on Conway polynomials without the lexicographic constraint that imposes uniqueness.

Apply:

Depends on #14958 Depends on #12142

CC: @defeo @rbeezer @sagetrac-hds @simon-king-jena @zimmermann6 @xcaruso @pjbruin @sagetrac-mraum @fredstro @sagetrac-JCooley @loefflerd @sagetrac-dfesti

Component: algebra

Keywords: days49 sd51

Author: David Roe, Jean-Pierre Flori, Peter Bruin

Reviewer: Jean-Pierre Flori, Luca De Feo

Merged: sage-5.13.beta1

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

jpflori commented 11 years ago
comment:37

Attachment: trac_8335-fixes-5.8.b0.patch.gz

Should really be ok now... sorry for the noise.

defeo commented 11 years ago
comment:39

Got the following failures

sage -t -long devel/sage/sage/coding/code_constructions.py # 90 doctests failed
sage -t -long devel/sage/doc/en/thematic_tutorials/group_theory.rst # 1 doctests failed
sage -t -long devel/sage/sage/modular/arithgroup/arithgroup_perm.py # 1 doctests failed
sage -t -long devel/sage/sage/groups/matrix_gps/matrix_group.py # 1 doctests failed
sage -t -long devel/sage/sage/homology/examples.py # 7 doctests failed

They seem unrelated, though, and disapperead upon second testing. I'll dig the code and try to give a review in the next weeks.

saraedum commented 11 years ago

Description changed:

--- 
+++ 
@@ -19,8 +19,6 @@

 __APPLY__

-*[attachment: trac_8335-pseudo_conway-5.8.b0.patch](https://github.com/sagemath/sage-prod/files/10648219/trac_8335-pseudo_conway-5.8.b0.patch.gz)
-
-*[attachment: trac_8335-finite_field_coerce-5.8.b0.patch](https://github.com/sagemath/sage-prod/files/10648220/trac_8335-finite_field_coerce-5.8.b0.patch.gz)
-
-*[attachment: trac_8335-fixes-5.8.b0.patch](https://github.com/sagemath/sage-prod/files/10648222/trac_8335-fixes-5.8.b0.patch.gz)
+1. [attachment: trac_8335-pseudo_conway-5.8.b0.patch](https://github.com/sagemath/sage-prod/files/10648219/trac_8335-pseudo_conway-5.8.b0.patch.gz)
+2. [attachment: trac_8335-finite_field_coerce-5.8.b0.patch](https://github.com/sagemath/sage-prod/files/10648220/trac_8335-finite_field_coerce-5.8.b0.patch.gz)
+3. [attachment: trac_8335-fixes-5.8.b0.patch](https://github.com/sagemath/sage-prod/files/10648222/trac_8335-fixes-5.8.b0.patch.gz)
saraedum commented 11 years ago
comment:40

Apply trac_8335-pseudo_conway-5.8.b0.patch trac_8335-finite_field_coerce-5.8.b0.patch trac_8335-fixes-5.8.b0.patch

jpflori commented 11 years ago
comment:42

Did anyone actually had the time to look at this? It would have been great to have that in Sage during the FLINT workshop last week to check some results using Sage rather than Magma.

I'm ok with what David initially did modulo what had to be fixed and that I hopefully fixed. So what's left to do is to review the rebasing and changes I introduced. Note that I did not check that the patches still cleanly apply, that may be an issue now.

saraedum commented 11 years ago
comment:43

Replying to @jpflori:

Did anyone actually had the time to look at this?

I started to look at it a while ago…

So what's left to do is to review the rebasing and changes I introduced.

That's good to know. I don't want to make any promises about reviewing this but I would certainly like to see this in sage. In any case, I won't review this in the next three weeks.

saraedum commented 11 years ago

Reviewer: Jean-Pierre Flori

jpflori commented 11 years ago
comment:44

Apart from fuzz 2, it still applies cleanly to 5.10.beta3 and all tests passes. Not sure what the patchbot complains about, missing docstrings?

jpflori commented 11 years ago

Changed author from David Roe to David Roe, Jean-Pierre Flori

jpflori commented 11 years ago

Description changed:

--- 
+++ 
@@ -19,6 +19,6 @@

 __APPLY__

-1. [attachment: trac_8335-pseudo_conway-5.8.b0.patch](https://github.com/sagemath/sage-prod/files/10648219/trac_8335-pseudo_conway-5.8.b0.patch.gz)
+1. [attachment: trac_8335-pseudo_conway-5.10.b3.patch](https://github.com/sagemath/sage-prod/files/10648223/trac_8335-pseudo_conway-5.10.b3.patch.gz)
 2. [attachment: trac_8335-finite_field_coerce-5.8.b0.patch](https://github.com/sagemath/sage-prod/files/10648220/trac_8335-finite_field_coerce-5.8.b0.patch.gz)
 3. [attachment: trac_8335-fixes-5.8.b0.patch](https://github.com/sagemath/sage-prod/files/10648222/trac_8335-fixes-5.8.b0.patch.gz)
saraedum commented 11 years ago
comment:46

Attachment: trac_8335-pseudo_conway-5.10.b3.patch.gz

Apply trac_8335-pseudo_conway-5.8.b0.patch trac_8335-finite_field_coerce-5.8.b0.patch trac_8335-fixes-5.10.b3.patch

saraedum commented 11 years ago
comment:47

apply trac_8335-pseudo_conway-5.10.b3.patch trac_8335-finite_field_coerce-5.8.b0.patch trac_8335-fixes-5.8.b0.patch

jpflori commented 11 years ago
comment:48

This definitely lacks "going down", e.g. taking the trace from a finite field to a subfield and being able to recognize that as an element of the subfield. But let's keep that for another ticket.

defeo commented 11 years ago

Changed keywords from none to days49

defeo commented 11 years ago

Changed reviewer from Jean-Pierre Flori to Jean-Pierre Flori, Luca De Feo

defeo commented 11 years ago
comment:49

Hi,

The doc in constructor.py says

 - ``modulus`` - blabla
   - 'default': a Conway polynomial is used if in the database. Otherwise 
     a sparse polynomial is used for binary fields and a 
     random polynomial is used for other characteristics.

but I got the impression that the default is to use pseudo-conway. By the way, is it reasonable to use pseudo-conway as default ? It is utterly slow !

sage: %time k = GF(next_prime(100000)^2) 
CPU times: user 16.41 s, sys: 0.05 s, total: 16.45 s
Wall time: 16.18 s
sage: %time l = GF(next_prime(100000)^3) 
CPU times: user 60.19 s, sys: 0.07 s, total: 60.26 s
Wall time: 59.97 s
sage: %time (k.gen() + l.gen()).parent()
CPU times: user 0.08 s, sys: 0.00 s, total: 0.09 s
Wall time: 0.07 s
Finite Field in z6 of size 100003^6

Compare this with Magma

> time k := GF(NextPrime(100000)^2);                                                                                                  
Time: 0.000
> time l := GF(NextPrime(100000)^3); 
Time: 0.000

> time CommonOverfield(l, k);              
Time: 0.030

Wouldn't it be better to keep using random as default, and give error messages suggesting to use conway when the user tries to add/multiply/whatever two elements in different fields?

On another note, I get the following messages (I suppressed them from the output above for readability).

sage: k = GF(next_prime(10000)^11)
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.

Any ideas on these?

jpflori commented 11 years ago
comment:50

Replying to @defeo:

Hi,

The doc in constructor.py says

 - ``modulus`` - blabla
   - 'default': a Conway polynomial is used if in the database. Otherwise 
     a sparse polynomial is used for binary fields and a 
     random polynomial is used for other characteristics.

but I got the impression that the default is to use pseudo-conway. By the way, is it reasonable to use pseudo-conway as default ? It is utterly slow !

It is and is expected to be.

sage: %time k = GF(next_prime(100000)^2) 
CPU times: user 16.41 s, sys: 0.05 s, total: 16.45 s
Wall time: 16.18 s
sage: %time l = GF(next_prime(100000)^3) 
CPU times: user 60.19 s, sys: 0.07 s, total: 60.26 s
Wall time: 59.97 s
sage: %time (k.gen() + l.gen()).parent()
CPU times: user 0.08 s, sys: 0.00 s, total: 0.09 s
Wall time: 0.07 s
Finite Field in z6 of size 100003^6

Compare this with Magma

> time k := GF(NextPrime(100000)^2);                                                                                                  
Time: 0.000
> time l := GF(NextPrime(100000)^3); 
Time: 0.000

> time CommonOverfield(l, k);              
Time: 0.030

I think the rationale is that Sage did not support finite fields with unnamed generator before this patch. So IIRC you could simply not do the above. If you do something which used to work like

K.<a> = GF(182918291829182918291892819)

(assuming the random typing is some prime power) I think it still picks up a random modulus as before (and is fast enough).

I agree it's kind of a bad thing to provide a new (but still natural, especially for Magma's users) way to create finite fields which is horribly slow. So we might want to fix this.

Also note that this ticket only provides lattices for finite fields created with pseudo-conway polynomials. The goal is not to provide (compatible) embeddings for all finite fields (as Magma is capable of).

Wouldn't it be better to keep using random as default, and give error messages suggesting to use conway when the user tries to add/multiply/whatever two elements in different fields?

On another note, I get the following messages (I suppressed them from the output above for readability).

sage: k = GF(next_prime(10000)^11)
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.
  ***   Warning: Mod(a,b)^n with n >> b : wasteful.

Any ideas on these?

No idea, I'll check tomorrow.

jpflori commented 11 years ago

Attachment: trac_8335-fixes-5.11.b1.2.patch.gz

jpflori commented 11 years ago
comment:51

There was some actual bug in the code which triggered the computation of the pseudo-Conway polynomials tree twice in the case where the extension degree was prime. First the way it should, and then using the same arguments as in the case where this degree is not prime which at some point triggered the computation of the power of modular integer with a small modulus and a huge exponent and PARI rants when you do that; just try

Mod(3,5)._pari_()**28172187218728127182718271821982918291829182918291

So the newly uploaded patch makes it so we only build the tree once as we always should have, and at least in Luca's example it prevents PARI rants to get on the screen.

jpflori commented 11 years ago

Description changed:

--- 
+++ 
@@ -21,4 +21,4 @@

 1. [attachment: trac_8335-pseudo_conway-5.10.b3.patch](https://github.com/sagemath/sage-prod/files/10648223/trac_8335-pseudo_conway-5.10.b3.patch.gz)
 2. [attachment: trac_8335-finite_field_coerce-5.8.b0.patch](https://github.com/sagemath/sage-prod/files/10648220/trac_8335-finite_field_coerce-5.8.b0.patch.gz)
-3. [attachment: trac_8335-fixes-5.8.b0.patch](https://github.com/sagemath/sage-prod/files/10648222/trac_8335-fixes-5.8.b0.patch.gz)
+3. [attachment: trac_8335-fixes-5.11.b1.patch](https://github.com/sagemath/sage-prod/files/10648225/trac_8335-fixes-5.11.b1.patch.gz)
jpflori commented 11 years ago

Work Issues: pari_warn, assetion error

jpflori commented 11 years ago
comment:52

If you try

GF(next_prime(10000)^12

which is a non-prime extension you still get PARI's rants on-screen.

More worrying is that

GF(next_prime(10000)^14)

fails with an AssertionError.

jpflori commented 11 years ago
comment:53

All these troubles seem to come from the _find_pow_of_frobenius function (the algo used inside and arguments passed to it).

jpflori commented 11 years ago
comment:54

New patch which pushouts elements of two extension fields of same characteristic into the extension field with degree the lcm rather than the product as the old code used to do.

jpflori commented 11 years ago
comment:55

Thanks to Luca I had a look at the math part of the algorithms implemented and am not sure about all of it. In particular, after a quick look, it seems to me that all the _frobenius_shift part is useless and all that should be implemented is the algorithm form HL99 without the last loop checking that the polynomial is indeed minimal for some order (e.g. the one used for conway polynomials).

jpflori commented 11 years ago
comment:56

I think I missed something in the algo so the above message is surely wrong, I need some more time...

jpflori commented 11 years ago
comment:57

I had another look at the proofs in the paper and the _frobenius_shift stuff definitely seems to make sense (on top of the fact that without it everything is broken :)), due to the fact we use actual concrete representations of the fields whereas the algorithm is somehow more abstract.

So now we're back with the problems from #8335 comment:52.

jpflori commented 11 years ago
comment:58

This already fails with:

find_pseudo_conway_polynomial_tree(5,6,False)
jpflori commented 11 years ago
comment:59

I think the problem is that we don't (or not anymore, there used to be two calls to the PCPT constructor before in the case where n is prime) ensure consistency of roots chosen for prime degree extension (basically running kind of _frobenius_shift when n is prime).

jpflori commented 11 years ago
comment:60

That was easily fixed.

Note that

find_pseudo_conway_polynomial_tree(11,14,False)

seems to enter an infinite loop.

jpflori commented 11 years ago
comment:61

Ok, it seems the main issue now is that nth_root is slow for big parameters (or make the range() function oveflow).

jpflori commented 11 years ago
comment:62

The ovrflow happens because I wanted to let nth_root return all roots rather than just one as in David's code.

Using David's approach let the calculation for

GF(next_prime(10000**14))

finish (although we still get PARI's warnings but that's "not really" a problem).

jpflori commented 11 years ago
comment:63

Ok, the set of patch looks quite clean now, so back to needs_review.

The only thing not perfect is that we still get PARI's warning in some cases...

jpflori commented 11 years ago

Changed work issues from pari_warn, assetion error to none

jpflori commented 11 years ago
comment:64

And back to needs_work, I did not check the doctests in the modified files.

jpflori commented 11 years ago
comment:65

Should be ok now. Reverted David's test for primitivity rather than using is_primitive which would waste a lot of time factoring over and over q = p**n -1.

jpflori commented 11 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-Implements coercion within lattices of finite fields lying above the same prime.
+Implements coercion within lattices of finite fields lying above the same prime when implemented with (pseudo-)Conway polynomials.

sage: k = GF(9)

jpflori commented 11 years ago
comment:67

(Small fix to a silly typo)

jpflori commented 11 years ago
comment:68

Added some doctests hopefully showing that embeddings are correct and compatible.

jpflori commented 11 years ago
comment:69

Ok, the last tests I added:


        sage: old_exists_cp = sage.rings.finite_rings.constructor.exists_conway_polynomial
        sage: sage.rings.finite_rings.constructor.exists_conway_polynomial = lambda p, n: False
        sage: k = GF(3**10)
        sage: l = GF(3**20)
        sage: k.modulus() == conway_polynomial(3, 10)
        False
        sage: l(k.gen()**10) == l(k.gen())**10
        True
        sage: del k, l
        sage: import gc
        sage: gc.collect();
        sage: sage.rings.finite_rings.constructor.exists_conway_polynomial = old_exists_cp

introduced some failures in the test:

sage: GF.other_keys(key, K)
            [(9, ('a',), x^2 + 2*x + 2, None, '{}', 3, 2, True),
             (9, ('a',), x^2 + 2*x + 2, 'givaro', '{}', 3, 2, True)]

Indeed, although I explicitely asked to delete k and l and restored a proper exists_... function, the field GF(320) stay cached and so the pseudo-Conway, but not Conway, modulus used to build GF(32).

The reason for that is that we performed arithmetic on elements of k and l which triggered creation of an embedding of k into l which prevents the collection of k and l. This is bad and is surely the same problem as in #14711.

So I just removed the hack forcing the use of pseudo-Conway polynomials.

xcaruso commented 11 years ago
comment:70

Attachment: trac_8335-fixes-5.11.b1.patch.gz

pjbruin commented 11 years ago
comment:71

In relation to #12142, I am currently working on a patch that makes the construction of irreducible polynomials independent of the finite field implementations (prime_modn, givaro, ntl_gf2e, ext_pari and the future pari_ffelt). Currently every finite field implementation has to do its own parsing of string values of modulus (like 'conway', 'random' etc.). It would be more elegant to handle all these string values in the generic constructor, and always pass an actual polynomial to the finite field implementation.

In the current version of Sage, the only situation in which the implementation needs to know whether the field is defined by a Conway polynomial is when converting elements to GAP. This means that always passing a polynomial as the modulus is fine, as long as one can check if the polynomial is a Conway polynomial if and when it is needed. This is easy to do by checking the database.

With your patch, each implementation gets an increased dependence on string values for the modulus parameter. I am wondering if this can be avoided by doing all the pseudo-Conway related stuff inside the generic finite field code.

[...to be continued...]

jpflori commented 11 years ago
comment:72

Replying to @pjbruin:

In relation to #12142, I am currently working on a patch that makes the construction of irreducible polynomials independent of the finite field implementations (prime_modn, givaro, ntl_gf2e, ext_pari and the future pari_ffelt). Currently every finite field implementation has to do its own parsing of string values of modulus (like 'conway', 'random' etc.). It would be more elegant to handle all these string values in the generic constructor, and always pass an actual polynomial to the finite field implementation.

In the current version of Sage, the only situation in which the implementation needs to know whether the field is defined by a Conway polynomial is when converting elements to GAP. This means that always passing a polynomial as the modulus is fine, as long as one can check if the polynomial is a Conway polynomial if and when it is needed. This is easy to do by checking the database.

Beware that the last patch which contains my modification to the two original patches by David changes quite a lot of thing. In particular, I think that with it modulus contains the real modulus as well and I use additional attributes to check for pseudo-Conway construction.

Also note that the real nice addition of this patch is mainly the compatible embeddings. Indeed, for practical purposes, it seems that only the Conway polynomials from the databse will be used. Constructing pseudo-Conway polynomials is quite as slow as contructions genuine Conway polynomials so when you will actually use them, i.e. for quite large parameters because you have to be outside of the Conway database, it will already be quite slow and unpractical.

With your patch, each implementation gets an increased dependence on string values for the modulus parameter. I am wondering if this can be avoided by doing all the pseudo-Conway related stuff inside the generic finite field code.

[...to be continued...]

pjbruin commented 11 years ago
comment:73

Replying to @jpflori:

Beware that the last patch which contains my modification to the two original patches by David changes quite a lot of thing. In particular, I think that with it modulus contains the real modulus as well and I use additional attributes to check for pseudo-Conway construction.

OK, I'll try to find out if and how my ideas about making the choice of polynomial independently of the finite field implementation can be harmonised with your patch. At first sight there does not seem to be a huge clash. I am mostly worried about the use of strings like 'conwayz' as a modulus, which really seems to be overloading this parameter too much.

Also note that the real nice addition of this patch is mainly the compatible embeddings.

I agree that this is a very desirable thing to have, and it is also nice to be able to simply type F = GF(p<sup>n</sup>) without having to care about variable names and embeddings. I do think it is good to think carefully about how exactly to accomplish this.

In particular, I am not sure if it is wise to say in the documentation/specification that this compatibility is achieved using (pseudo-)Conway polynomials, since different implementations are imaginable. I am thinking of the standard models for finite fields by Lenstra and de Smit, which are constructed in a way that does not seem to be related to Conway polynomials.

From a conceptual point of view, it is desirable that GF(pn) without any arguments should refer to the unique subfield of cardinality pn inside a chosen algebraic closure of GF(p). This gives 'compatible embeddings' the very simple meaning of 'inclusions within an algebraic closure'.

Such an algebraic closure could be implemented in different ways, for example via (pseudo-)Conway polynomials; algebraic closures of GF(p) resulting from different methods would be non-canonically isomorphic. There might be a default choice that could change in the future, and the user should be able to specify which algebraic closure should be taken.

Here is how I would hope a hypothetical future Sage session to look like:

sage: p = 5
sage: Fpbar = GF(p).algebraic_closure()
sage: Fpbar
Algebraic closure of Finite Field of size 5
sage: Fa = GF(p^2, 'a')
sage: Fa
Finite field in a of size 5^2
sage: is_subfield(Fa, Fpbar)
False
sage: Fb = GF(p^2)
Subfield of size 5^2 of Algebraic closure of Finite Field of size 5
sage: is_subfield(Fb, Fpbar)
True
sage: type(Fpbar)
<class 'sage.rings.AlgebraicClosureFiniteField_pseudo_conway'>

Would something like this be easy to achieve once this ticket has been implemented?

jpflori commented 11 years ago
comment:74

Replying to @pjbruin:

Replying to @jpflori:

Beware that the last patch which contains my modification to the two original patches by David changes quite a lot of thing. In particular, I think that with it modulus contains the real modulus as well and I use additional attributes to check for pseudo-Conway construction.

OK, I'll try to find out if and how my ideas about making the choice of polynomial independently of the finite field implementation can be harmonised with your patch. At first sight there does not seem to be a huge clash. I am mostly worried about the use of strings like 'conwayz' as a modulus, which really seems to be overloading this parameter too much.

Also note that the real nice addition of this patch is mainly the compatible embeddings.

I agree that this is a very desirable thing to have, and it is also nice to be able to simply type F = GF(p<sup>n</sup>) without having to care about variable names and embeddings. I do think it is good to think carefully about how exactly to accomplish this.

Agreed. But it just happened I stumled upon this ticket which already looked usable (and was two years old) and thought "oh, let's already get this in; later on we can design better constructions of compatible embeddings and get something more general and fast". So I decided to postpone the design of a cool and fast system in #8751 and only deal with "lattices using (pseudo) Conway polynomials" here. It's better to have something than nothing.

In particular, I am not sure if it is wise to say in the documentation/specification that this compatibility is achieved using (pseudo-)Conway polynomials, since different implementations are imaginable. I am thinking of the standard models for finite fields by Lenstra and de Smit, which are constructed in a way that does not seem to be related to Conway polynomials.

I completely agree that using Conway polynomials is a no go as soon as you quit fields of small cardinalities. I've met Eric Rains who participated in the Magma implementation (or at least the algos behind it) and he was nice enough to share with me a draft describing the algos used.

De Feo and Schost and others are also producing nice paper on how to build p- or l-adic towers of finite fields. What is very nice is that they avoid linear algebra (what Magma may not completely do).

From a conceptual point of view, it is desirable that GF(pn) without any arguments should refer to the unique subfield of cardinality pn inside a chosen algebraic closure of GF(p). This gives 'compatible embeddings' the very simple meaning of 'inclusions within an algebraic closure'.

Such an algebraic closure could be implemented in different ways, for example via (pseudo-)Conway polynomials; algebraic closures of GF(p) resulting from different methods would be non-canonically isomorphic. There might be a default choice that could change in the future, and the user should be able to specify which algebraic closure should be taken.

I agree, so it makes sense to have a pseudo Conway implementation and other ones later.

Here is how I would hope a hypothetical future Sage session to look like:

sage: p = 5
sage: Fpbar = GF(p).algebraic_closure()
sage: Fpbar
Algebraic closure of Finite Field of size 5
sage: Fa = GF(p^2, 'a')
sage: Fa
Finite field in a of size 5^2
sage: is_subfield(Fa, Fpbar)
False
sage: Fb = GF(p^2)
Subfield of size 5^2 of Algebraic closure of Finite Field of size 5
sage: is_subfield(Fb, Fpbar)
True
sage: type(Fpbar)
<class 'sage.rings.AlgebraicClosureFiniteField_pseudo_conway'>

Would something like this be easy to achieve once this ticket has been implemented?

pjbruin commented 11 years ago
comment:75

I started to look at the patches, but was immediately struck by a problem that has nothing to do with finite fields. In QuotientFunctor._apply_functor, the functor R -> R/IR (where I is an ideal of the base ring) to arbitrary rings. This makes perfect sense for any R; you just happen to get the zero ring when IR = R. The existing behaviour is certainly correct (although it is debatable whether the zero ring should be represented as Integers(1)). Why would you want to raise an exception if R is a field?

jpflori commented 11 years ago
comment:76

I think it was the historical Sage behavior until some recent ticket (don't really remember, you should be able to devise which one by looking at the hg log).

I also agree returning the zero ring would be a better thing to do. But then it breaks a bunch of generic constructions in Sage such as polynomial rings over {0} and quotients of it... See #8335 comment:24 and the few following comments (in case you manage to make some sense of my lonely rants).

pjbruin commented 11 years ago
comment:77

Replying to @jpflori:

I think it was the historical Sage behavior until some recent ticket (don't really remember, you should be able to devise which one by looking at the hg log).

In the comments it says that the behaviour used to be to return the integer 0 (?!), and that this was corrected in #9138. Now that the current implementation is correct, it would be very bad to change something fundamental like this just to make new patches work.

I also agree returning the zero ring would be a better thing to do. But then it breaks a bunch of generic constructions in Sage such as polynomial rings over {0} and quotients of it... See #8335 comment:24 and the few following comments (in case you manage to make some sense of my lonely rants).

Then it seems to me that the generic constructions should be fixed. Until then, any new code should take care that it does not use these constructions in cases where they fail.

jpflori commented 11 years ago
comment:78

Replying to @pjbruin:

Replying to @jpflori:

I think it was the historical Sage behavior until some recent ticket (don't really remember, you should be able to devise which one by looking at the hg log).

In the comments it says that the behaviour used to be to return the integer 0 (?!), and that this was corrected in #9138. Now that the current implementation is correct, it would be very bad to change something fundamental like this just to make new patches work.

I also agree returning the zero ring would be a better thing to do. But then it breaks a bunch of generic constructions in Sage such as polynomial rings over {0} and quotients of it... See #8335 comment:24 and the few following comments (in case you manage to make some sense of my lonely rants).

Then it seems to me that the generic constructions should be fixed. Until then, any new code should

take care that it does not use these constructions in cases where they fail. Of course. But avoiding the coercion model is not that easy.

I'll give it a shot, but I cannot promise to come up with anything working. Obviously I would have done that earlier if it was really easy.

jpflori commented 11 years ago
comment:79

In fact I think that putting back the correct behavior gives no problems and I added all needed fixes to make it work.

The real problem was that the quotient functor was applied before the fraction field one or the other way around and you always ended up working in the trivial ring. So the easy way was raising an error. But now the functors are applied in a more sensible it should not be a problem.

I'll upload a fixed patch when I make sure no tests fail.