Opened 5 years ago

Last modified 17 months ago

#24526 needs_work defect

more user friendly finite field extensions

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-8.2
Component: number theory Keywords:
Cc: Merged in:
Authors: Vincent Delecroix Reviewers:
Report Upstream: N/A Work issues: merge conflict
Branch: u/vdelecroix/24526 (Commits, GitHub, GitLab) Commit: ba8454698b6ebc00afcebeefc5b586e2dfcff4be
Dependencies: Stopgaps:

Status badges

Description (last modified by vdelecroix)

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.

Change History (7)

comment:1 Changed 5 years ago by vdelecroix

  • Authors set to Vincent Delecroix
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 5 years ago by vdelecroix

  • Branch set to u/vdelecroix/24526
  • Commit set to ba8454698b6ebc00afcebeefc5b586e2dfcff4be

New commits:

ba8454624526: more user friendly finite field extension

comment:3 Changed 5 years ago by mmezzarobba

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.

comment:4 Changed 4 years ago by darij

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').

comment:5 Changed 3 years ago by dkrenn

  • Status changed from needs_review to needs_work
  • Work issues set to merge conflict

Does not apply anymore.

comment:6 Changed 17 months ago by slelievre

Recent user report for the same problem:

comment:7 Changed 17 months ago by slelievre

Please rebase, and I can review.

Note: See TracTickets for help on using tickets.