sagemath / sage

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

more user friendly finite field extensions #24526

Open videlec opened 6 years ago

videlec commented 6 years ago

GF(p).extension(xxx) does not accept symbolic polynomials... and the error is cryptic

sage: Fp = GF(3)
sage: Fp2.<i> = Fp.extension(x^2+1)
Traceback (most recent call last):
...
UnboundLocalError: local variable 'E' referenced before assignment

We make it more user friendly by allowing the above in some duck typing fashion.

Component: number theory

Work Issues: merge conflict

Author: Vincent Delecroix

Branch/Commit: u/vdelecroix/24526 @ ba84546

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

videlec commented 6 years ago

Author: Vincent Delecroix

videlec commented 6 years ago

Description changed:

--- 
+++ 
@@ -7,3 +7,5 @@
 ...
 UnboundLocalError: local variable 'E' referenced before assignment

+ +We make it more user friendly by allowing the above in some duck typing fashion.

videlec commented 6 years ago

New commits:

ba8454624526: more user friendly finite field extension
videlec commented 6 years ago

Commit: ba84546

videlec commented 6 years ago

Branch: u/vdelecroix/24526

mezzarobba commented 6 years ago
comment:3

It may be better to keep the branch dealing with tuples/lists for speed reasons... Perhaps it doesn't matter (I don't know how much it is used or what alternatives exist for constructing extensions without too much overhead), but your patch makes the call to extension() in GF(3).extension([1, 0, 1], 'a') 70% slower on my system.

darijgr commented 6 years ago
comment:4

Should this ducktyping really all be happening under the hood without notifying the user? I think a more readable error message would be better, or at least a warning. We don't want people to use SR for polynomials in their code, do we?

Also, is this

+                else:
+                    modulus = modulus.change_ring(self)

necessary? The GF constructor already does modulus = R(modulus).monic(), where R = PolynomialRing(FiniteField(p), 'x').

dkrenn commented 5 years ago
comment:5

Does not apply anymore.

dkrenn commented 5 years ago

Work Issues: merge conflict

slel commented 3 years ago
comment:6

Recent user report for the same problem:

slel commented 3 years ago
comment:7

Please rebase, and I can review.