fix coercion from libgap's finite fields, use libgap in sage/rings/finite_rings

[dimpase]

implement coercion from libgap's finite fields, to fix e.g.

sage: F=GF(25)
sage: F(libgap.Z(25)^3)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
...
TypeError: unable to coerce <class 'sage.libs.gap.element.GapElement_FiniteField'>

With the ticket branch, this works:

sage: F=GF(25)
sage: F(libgap.Z(25)^3)
4*z2 + 3

---

As well, we switch to use libgap instead of pexpect GAP - internally
in the affected files. This is a part of #26902

CC: @nbruin @mkoeppe @kwankyu

Component: number theory

Author: Dima Pasechnik

Branch/Commit: 6e5ea79

Reviewer: Matthias Koeppe

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

[dimpase]

Commit: 02bd544

[dimpase]

New commits:

02bd544

fix coersion of libgap FFelements; switch to libgap

[dimpase]

Branch: u/dimpase/rings/FFEs_to_accept_libgap

[dimpase]

comment:4

I've left intact the ability to accept pexpect GAP data to be converted in Sage finite field elements - this means I need a test for this type.
I'm using an import statement

from sage.interfaces.gap import is_GapElement

and a call to is_GapElement().
Is there a way to avoid this call? I can also put the import into the try block and
set is_GapElement() to always return False in case of import exception.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

21e38a7

fix coersion of libgap FFelements; switch to libgap

[sagetrac-git]

Changed commit from 02bd544 to 21e38a7

[sagetrac-git]

Changed commit from 21e38a7 to d4bde80

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

d4bde80

make imports of is_GapEelment uniform across files

[dimpase]

comment:7

I can also resort to not calling is_GapElement(), and rely on exceptions - this would need a bit more rearranging of if/else.

However, there are still explicit tests like sage: S(gap('Z(25)^3')), and there is a discrepancy between libgap and (pexpect) gap in the way they treat such inputs:

sage: gap('Z(25)^3')
Z(5^2)^3
sage: gap.eval('Z(25)^3')
'Z(5^2)^3'
sage: libgap.eval('Z(25)^3')
Z(5^2)^3
sage: libgap('Z(25)^3')
"Z(25)^3"

So these tests will have to go as soon as gap is set to libgap.

[kwankyu]

comment:9

I am not familiar with libgap and less with GAP...

This gfq_gap_to_sage(e, self.parent) runs faster than libgap(e).sage(ring=self.parent) with me. Then why do you replace the former with the latter? Removing pexpect gap is the primary objective?

[dimpase]

comment:10

Replying to Kwankyu Lee:

This gfq_gap_to_sage(e, self.parent) runs faster than libgap(e).sage(ring=self.parent) with me.

Loading libgap takes time. After it's loaded, it should actually be faster to use
it.

In this particular case, you only need libgap(e) on e coming from pexpect gap.
So there is an extra conversion involved, and we are getting rid of pexpect gap
anyway, so after this is done these conversions (to Sage types) will be much faster
(they are just e.sage(ring=self.parent))

One should not use pexpect gap, it should be deprecated. Do you want this to happen on this ticket?

[dimpase]

comment:11

the only "real" use of gfq_gap_to_sage is in sage/coding - and this is eliminated in #34771, along with the use of pexpect GAP.

Thus the only possible speed regression can appear in user code, while using pexpect GAP.

[kwankyu]

comment:12

Replying to Dima Pasechnik:

I'm using an import statement

from sage.interfaces.gap import is_GapElement

and a call to is_GapElement().
Is there a way to avoid this call? I can also put the import into the try block and
set is_GapElement() to always return False in case of import exception.

Why is there a possibility of import exception?

The definition of is_GapElement() is isinstance(x, GapElement). This lets you avoid the call. But perhaps I simply don't understand your intention or context...

[dimpase]

comment:13

Replying to Kwankyu Lee:

The definition of is_GapElement() is isinstance(x, GapElement). This lets you avoid the call.

Great, this will simplify things a bit. It didn't cross my mind that is_GapElement()
is that simple.

[dimpase]

comment:14

Replying to Dima Pasechnik:

Replying to Kwankyu Lee:

The definition of is_GapElement() is isinstance(x, GapElement). This lets you avoid the call.

Great, this will simplify things a bit. It didn't cross my mind that is_GapElement()
is that simple.

well, not really, as one still need to import GapElement (Cython won't even compile without it)

[kwankyu]

comment:15

Do you want to avoid importing from sage.interfaces.gap? Why?

[mkoeppe]

comment:16

If this is for modularization purposes in which an import may fail: I am using this idiom for such isinstance tests:

try:
    from sage.interfaces.gap import GapElement
except ImportError:
    GapElement = ()

... isinstance(x, GapElement)

[dimpase]

comment:17

Replying to Kwankyu Lee:

Do you want to avoid importing from sage.interfaces.gap? Why?

as a preparation for deprecation and removal. We really don't need
a pexpect interface to GAP.

[dimpase]

comment:18

Replying to Matthias Köppe:

If this is for modularization purposes in which an import may fail: I am using this idiom for such isinstance tests:

It's for coming deprecation and removal, seems to do the job here, too.

[mkoeppe]

comment:19

Another approach: #34804 Deprecate sage.interfaces is_...Element functions

[mkoeppe]

comment:20

Replying to Dima Pasechnik:

Replying to Matthias Köppe:

If this is for modularization purposes in which an import may fail: I am using this idiom for such isinstance tests:

It's for coming deprecation and removal, seems to do the job here, too.

For deprecation it will not work so well because modules such as sage.interfaces.gap create an instance of the interface class at load time, which would trigger the deprecation warning already.

[kwankyu]

comment:21

Replying to Dima Pasechnik:

Replying to Kwankyu Lee:

Do you want to avoid importing from sage.interfaces.gap? Why?

as a preparation for deprecation and removal. We really don't need
a pexpect interface to GAP.

Okay. So this ticket is not removing pexpect GAP, but just drying it out(not sure if this phrase conveys the meaning). This seems to force you to write clumsy code.

How about adding only deprecation warnings without touching actual pexpect GAP code in this ticket, and then remove all of it in another ticket after deprecation period?

[dimpase]

comment:23

Replying to Matthias Köppe:

Another approach: #34804 Deprecate sage.interfaces is_...Element functions

OK, this is interesting. So this would mean unconditionally importing GapElement from
sage.interfaces.abs, right? I'm only not completely sure I understand what should be in the latter, as examples such as in src/sage/rings/abc.pyx look like dark magic to me.

[mkoeppe]

comment:24

Replying to Dima Pasechnik:

Replying to Matthias Köppe:

Another approach: #34804 Deprecate sage.interfaces is_...Element functions

OK, this is interesting. So this would mean unconditionally importing GapElement from
sage.interfaces.abs, right?

Yes.

I'm only not completely sure I understand what should be in the latter, as examples such as in src/sage/rings/abc.pyx look like dark magic to me.

I've pushed a barebones version to #34804. src/sage/rings/abc.pyx is longer, but it's just decoration

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

eb72ef0	avoid is_GapElement
1c4a69f	use GapElement from an abstract superclass

[sagetrac-git]

Changed commit from d4bde80 to 1c4a69f

[dimpase]

comment:26

OK, how about this? The branch of #34804 should be based on this one.

[mkoeppe]

comment:27

Yes, looking good.

[dimpase]

comment:28

review?

[mkoeppe]

comment:29

In this change:

--- a/src/sage/rings/finite_rings/element_ntl_gf2e.pyx
+++ b/src/sage/rings/finite_rings/element_ntl_gf2e.pyx
@@ -56,6 +54,11 @@ from .finite_field_ntl_gf2e import FiniteField_ntl_gf2e
 
 from sage.rings.polynomial.polynomial_ring_constructor import PolynomialRing
 
+from sage.libs.gap.element import GapElement_FiniteField
+

it would be good to use the try except ... () so that there is no hard load-time dependency on libgap.

[dimpase]

comment:30

Replying to Matthias Köppe:

In this change:

--- a/src/sage/rings/finite_rings/element_ntl_gf2e.pyx
+++ b/src/sage/rings/finite_rings/element_ntl_gf2e.pyx
@@ -56,6 +54,11 @@ from .finite_field_ntl_gf2e import FiniteField_ntl_gf2e
 
 from sage.rings.polynomial.polynomial_ring_constructor import PolynomialRing
 
+from sage.libs.gap.element import GapElement_FiniteField
+

it would be good to use the try except ... () so that there is no hard load-time dependency on libgap.

there are more importings of GapElement_FiniteField in this patch, i suppose you'd like to deal with them all.

Why don't we instead go the way we went with GapElement here, by adding sage.libs.abc, etc?

[mkoeppe]

comment:31

Replying to Dima Pasechnik:

Why don't we instead go the way we went with GapElement here, by adding sage.libs.abc, etc?

That's also fine

[dimpase]

comment:32

Replying to Matthias Köppe:

Replying to Dima Pasechnik:

Why don't we instead go the way we went with GapElement here, by adding sage.libs.abc, etc?

That's also fine

Can we do sage.libs.abc as a part of #32414, on a separate ticket?
After all, the current ticket is a part of #26902, which is not modularisation.

[mkoeppe]

comment:33

It's kind of why I suggested to use try except ... () in comment:30.

[dimpase]

comment:34

Replying to Matthias Köppe:

It's kind of why I suggested to use try except ... () in comment:30.

but we are not doing modularisation of libgap module here. that's why I don't want to try: here.

[mkoeppe]

comment:35

The ticket is introducing a modularization regression by inserting this new module-level import.

[dimpase]

comment:36

Replying to Matthias Köppe:

The ticket is introducing a modularization regression by inserting this new module-level import.

it replaces one modularization regression with another.

There are about 180 from sage.libs.gap... in Sage, I suppose none of them in try... except block. It's begging for a separate ticket, which is best attended to once we've replaced all pexpect GAP with libgap.

[mkoeppe]

comment:37

There are no unprotected module-level imports from sage.libs.gap in sage.rings. Please don't introduce one, that's all I'm asking.

[dimpase]

comment:38

Replying to Matthias Köppe:

There are no unprotected module-level imports from sage.libs.gap in sage.rings. Please don't introduce one, that's all I'm asking.

Do you mean it's OK for you to have imports in functions, say?
E.g. in src/sage/rings/number_field/number_field.py there is
def _libgap_(self) where libgap is unconditionally imported.

[sagetrac-git]

Changed commit from 1c4a69f to 6e5ea79

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

6e5ea79

move import of libgap into a function

[mkoeppe]

comment:40

Yes, that's https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#module-level-runtime-dependencies bullet point 2 "Replace module-level imports by method-level imports."

[mkoeppe]

Reviewer: Matthias Koeppe

[mkoeppe]

comment:41

LGTM, thanks.

[vbraun]

Changed branch from u/dimpase/rings/FFEs_to_accept_libgap to 6e5ea79