Improve FFE code a bit and add tests #2690
Conversation
fingolfin
added
kind: enhancement
Codecov Report
@@ Coverage Diff @@
## master #2690 +/- ##
==========================================
+ Coverage 75.43% 75.48% +0.04%
==========================================
Files 478 478
Lines 241610 241616 +6
==========================================
+ Hits 182259 182378 +119
+ Misses 59351 59238 -113
|
fingolfin
force-pushed
the
mh/ffe
branch
2 times, most recently
from
463c674
to
d1eddbf
Compare
fingolfin
changed the title
* add more FFE test cases, now finfield.c has almost full coverage * simplify `FuncLOG_FFE_DEFAULT` input validation * remove some comments pointing out problem when extending FFV to UInt4: we don't plan to do that (and it'd be impractical in the current setup anyway), and even if: those comments are not really helpful * make Z(p,d) stricter about verifying that `p` is a prime; and fixing an edge case on 32 bit systems, where Z(65536,2) lead to an overflow (which luckily had no consequences whatsoever) * adjust the error message of the kernel function `PowFFEFFE` (for "conjugating" FFE elements) to match those for +, -, *, / in case of arguments which have different characteristic. * fix bug in RootOfDefiningPolynomial
| @@ -600,7 +600,7 @@ InstallMethod( RootOfDefiningPolynomial, | |||
|
|
|||
| coeffs:= ShiftedCoeffs( coeffs[1], coeffs[2] ); | |||
| p:= Characteristic( F ); | |||
| d:= ( Length( coeffs ) - 1 ) * DegreeOverPrimeField( F ); | |||
| d:= DegreeOverPrimeField( F ); | |||
There was a problem hiding this comment.
Any ideas why this factor was there before?
There was a problem hiding this comment.
Good question. I dug a bit. That line was added by @ThomasBreuer in a commit from 1998-11-16 (hash 8a81e3cac9f5878891017aa3b6e9d060a0845f0d in the private git history). Looking at the code added there, a large part of it is identical to code in FieldExtension, including the bit computing d -- there, however, F was the base field, while here it is the extension field. So I suspect this was a copy&paste accident. Since this code was never triggered before I added the patches in this PR, we never noticed.
There was a problem hiding this comment.
One may in fact wonder whether it wouldn't be better to extract the common code into a new helper operationRootOfPolynomial(F, poly). Then the above RootOfDefiningPolynomial method would essentially becomes F -> RootOfPolynomial(PrimeField(F), DefiningPolynomial(F), and the FieldExtension method could also be shortened. And if somebody comes up with a better implementation than "try all field elements" to do this, they'd have to edit only one spot.
FuncLOG_FFE_DEFAULTinput validationdon't plan to do that (and it'd be impractical in the current setup anyway),
and even if: those comments are not really helpful
pis a prime; and fixing an edgecase on 32 bit systems, where Z(65536,2) lead to an overflow (which luckily
had no consequences whatsoever)
PowFFEFFE(for"conjugating" FFE elements) to match those for +, -, *, / in case of
arguments which have different characteristic.
One change in here might be controversial: I add calls to
IsPrimeIntintoZOpfor large primes. These could in theory cause a slow down, although, since we cache results of primality checks, usually that should not be noticeable. But it prevents stupid mistakes. So I'd argue it's no problem after all.