#12173: Update FLINT to 2.3
---------------------------------------------------------------------+------
Reporter: mhansen |
Owner: tbd
Type: enhancement |
Status: needs_work
Priority: major |
Milestone: sage-5.10
Component: packages: standard |
Resolution:
Keywords: flint spkg | Work
issues:
Report Upstream: N/A |
Reviewers: John Cremona, Jeroen Demeyer
Authors: Mike Hansen, Fredrik Johansson, Jean-Pierre Flori |
Merged in:
Dependencies: sage-5.9.beta5 |
Stopgaps:
---------------------------------------------------------------------+------
Comment (by jpflori):
Replying to [comment:311 burcin]:
> Here are some comments after reading
attachment:trac_12173-fixes-v6.patch.
>
> I don't understand this hunk:
>
> {{{
> diff --git a/sage/libs/flint/fmpz_poly.pyx
b/sage/libs/flint/fmpz_poly.pyx
> --- a/sage/libs/flint/fmpz_poly.pyx
> +++ b/sage/libs/flint/fmpz_poly.pyx
> @@ -53,7 +53,7 @@
> cdef long c
> cdef Integer w
> if PY_TYPE_CHECK(v, str):
> - if fmpz_poly_set_str(self.poly, v):
> + if not fmpz_poly_set_str(self.poly, v):
> return
> else:
> raise ValueError, "Unable to create Fmpz_poly from that
string."
> }}}
Indeed, this looks wrong.
This also shows this function does not seem to be well tested.
>
>
> `nmod_poly_pow(..., 2)` should be used below instead of
`nmod_poly_mul()`.
>
> {{{
> diff --git a/sage/libs/flint/nmod_poly_linkage.pxi
b/sage/libs/flint/nmod_poly_linkage.pxi
> --- a/sage/libs/flint/nmod_poly_linkage.pxi
> +++ b/sage/libs/flint/nmod_poly_linkage.pxi
> @@ -496,7 +495,7 @@
> elif e == 1:
> nmod_poly_set(res, x)
> elif e == 2:
> - nmod_poly_sqr(res, x)
> + nmod_poly_mul(res, x, x)
> else:
> if res == x:
> nmod_poly_set(tmp, x)
> @@ -510,7 +509,7 @@
> nmod_poly_set_coeff_ui(res, 0, 1)
> e = e >> 1
> while(e != 0):
> - nmod_poly_sqr(pow2, pow2)
> + nmod_poly_mul(pow2, pow2, pow2)
> if e % 2:
> nmod_poly_mul(res, res, pow2)
> e = e >> 1
> }}}
sure, but I wouldn't say that's a critical issue.
>
> This seems to remove the fast exit path:
>
> {{{
> diff --git a/sage/rings/fraction_field_FpT.pyx
b/sage/rings/fraction_field_FpT.pyx
> --- a/sage/rings/fraction_field_FpT.pyx
> +++ b/sage/rings/fraction_field_FpT.pyx
> @@ -688,26 +688,34 @@
> """
> if nmod_poly_degree(self._numer) == -1:
> return self
> - if not nmod_poly_sqrt_check(self._numer) or not
nmod_poly_sqrt_check(self._denom):
> - return None
> }}}
>
> Fredrik says that FLINT also does the same checks in `nmod_poly_sqrt()`,
but some memory allocations are performed by the time we get to that
point. If the input to this function is not a perfect square most of the
time, then IMHO removing these checks will introduce a performance
regression.
I think the best place for such a function would be in FLINT itself.
The problem is that if we plan on integrating this ticket without calling
this to be implemented function (I'd prefer to go this way, let's not wait
for another couple of monthes/years to merge this ticket), we should make
sure to not forget to call it from Sage when it actually gets implemented
in (a stable release of) FLINT, so we should surely open a follow-up
ticket.
>
> In the same hunk as above:
>
> {{{
> + # Make denominator monic
> + a = nmod_poly_leading(denom)
> + if a != 1:
> + a = mod_inverse_int(a, self.p)
> + nmod_poly_scalar_mul_nmod(numer, numer, a)
> + nmod_poly_scalar_mul_nmod(denom, denom, a)
> + # Choose numerator with smaller leading coefficient
> + a = nmod_poly_leading(numer)
> + if a > self.p - a:
> + nmod_poly_neg(numer, numer)
> }}}
>
> Is this necessary? If the input is normalized, the denominator will have
1 as a leading coefficient anyway. If we really want to normalize, the
`normalize()` method in the same file should be used.
Sure, I'll have alook at the code.
>
>
> `_nmod_poly_set_coeff_ui()` was removed from FLINT2. Removing the
underscore introduces a potential branch for each function call. Shall we
implement our own underscore version?
>
I'd say no, if that's useful, it should be in FLINT.
> {{{
> diff --git a/sage/rings/polynomial/polynomial_zmod_flint.pyx
b/sage/rings/polynomial/polynomial_zmod_flint.pyx
> --- a/sage/rings/polynomial/polynomial_zmod_flint.pyx
> +++ b/sage/rings/polynomial/polynomial_zmod_flint.pyx
> @@ -157,17 +170,47 @@
> <snip>
> + sig_on()
> for i from 0 <= i < length:
> - _nmod_poly_set_coeff_ui(&self.x, i, l_in[i])
> - # we need to set the length manually, we used
_nmod_poly_set_coeff_ui
> - self.x.length = length
> + nmod_poly_set_coeff_ui(&self.x, i, l_in[i])
> + sig_off()
> + return 0
> }}}
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/12173#comment:312>
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 unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
