#11685: Pari finite field extension: element created by list not recognised as
zero
----------------------------------------+-----------------------------------
Reporter: johanbosman | Owner: AlexGhitza
Type: defect | Status: needs_work
Priority: major | Milestone: sage-4.7.2
Component: algebra | Keywords: finite fields, pari
Work_issues: | Upstream: N/A
Reviewer: Simon King, Johan Bosman | Author: Johan Bosman, Jeroen
Demeyer
Merged: | Dependencies:
----------------------------------------+-----------------------------------
Changes (by SimonKing):
* status: needs_review => needs_work
Comment:
First of all, note that the option `check==False` is used, as you can see
by studying the arithmetic operations on finite field elements. And it is
clearly beneficial! For example:
Without patches, I get
{{{
sage: K.<a> = GF(5^9)
sage: b = 4*a^7 + 2*a^5 + 4*a^4 + 4*a^3 + 3*a^2 + 3*a
sage: %timeit b+3 # here, you will find that "check=False".
625 loops, best of 3: 30.9 µs per loop
sage: %timeit copy(b)
625 loops, best of 3: 14.3 µs per loop
}}}
With your patch, it takes 42 µs resp. 25.9 µs per loop.
25-30% difference is relevant. Thus, I'd prefer to keep the "check=False"
code.
Moreover, I can still not understand your rationale for removing the code:
Replying to [comment:32 jdemeyer]:
> 1) That option and the code was completely undocumented and the purpose
is not clear.
If code is not documented then it should be documented, not removed.
The purpose is very clear: Provide better speed by avoiding any tests (or
conversions, in this case). I think it is fairly common to have that
option in Sage, of course for internal use only.
The price to pay for the speed is that one needs to provide very specific
input: It must be pari.
> 2) The logic seems backward: executing more code when ''not'' checking?
You can't be serious.
If one has `check=False` then one just has
{{{
if not check:
if not value: # see comment below about this workaround
self.__value =
pari(0).Mod(parent._pari_modulus())*parent._pari_one()
else:
self.__value = value
return
}}}
and nothing more: Note the "return" in the last line.
> 3) The ``check`` code changes the functionality of the function, which
is usually not desired.
It is quite usual that the check option does change the behaviour of the
function, in the sense that a very specific input is expected, and it is
not tested whether the user really got it right.
Here, check=False means that the input must be an element in the pari
interface. Only since "zero" is a special case (see comments in the code),
you have the test "if not value". As a side effect, the input `[]` or
`None` or `False` would result in a zero element as well.
> To me checking should be something read-only, checking for invalid
input and throwing exceptions. But not modifying the input.
Sometimes you have two separate options `check` and `coerce`. Here,
indeed, checking means that the input is also converted, when necessary.
`convert_to_pari` might be a better name than `check`.
But since it is for internal use anyway, I see not much reason to change
the name.
> 4) the ``check`` code is related to the handling of zero inputs which is
anyway handled separately in the code below.
No. The check code simply provides a short cut for pari input. Only since
pari is inconsistent in the way how different types of zero are dealt
with, a special case is needed.
I suggest to use your patch without removing the `if check:` part. Can you
please update it? And since the arguments of the `__init__` method are not
documented at all, I suggest that I create a referee patch that fixes the
documentation.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/11685#comment:33>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
