fix coercion from libgap's finite fields, use libgap in sage/rings/finite_rings
Closed this issue · 48 comments
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
Component: number theory
Author: Dima Pasechnik
Branch/Commit: 6e5ea79
Reviewer: Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/34770
Description changed:
---
+++
@@ -12,8 +12,8 @@
With the ticket branch, this works:
```
-sage: sage: F=GF(25)
-sage: sage: F(libgap.Z(25)^3)
+sage: F=GF(25)
+sage: F(libgap.Z(25)^3)
4*z2 + 3
```
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.
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 |
Branch pushed to git repo; I updated commit sha1. New commits:
d4bde80 | make imports of is_GapEelment uniform across files |
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.
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?
Replying to Kwankyu Lee:
This
gfq_gap_to_sage(e, self.parent)runs faster thanlibgap(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?
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.
Description changed:
---
+++
@@ -1,4 +1,4 @@
-implement coersion from libgap's finite fields, to fix e.g.
+implement coercion from libgap's finite fields, to fix e.g.
```
sage: F=GF(25)Replying to Dima Pasechnik:
I'm using an import statement
from sage.interfaces.gap import is_GapElementand a call to
is_GapElement().
Is there a way to avoid this call? I can also put the import into thetryblock and
setis_GapElement()to always returnFalsein 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...
Replying to Kwankyu Lee:
The definition of
is_GapElement()isisinstance(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.
Replying to Dima Pasechnik:
Replying to Kwankyu Lee:
The definition of
is_GapElement()isisinstance(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)
Do you want to avoid importing from sage.interfaces.gap? Why?
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)
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.
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.
Another approach: #34804 Deprecate sage.interfaces is_...Element functions
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.
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?
By the way, there is also an interesting import hell issue I encountered here:
I'm changing here 4 files
src/sage/rings/finite_rings/element_givaro.pyx
src/sage/rings/finite_rings/element_ntl_gf2e.pyx
src/sage/rings/finite_rings/element_pari_ffelt.pyx
src/sage/rings/finite_rings/integer_mod_ring.py
Of them, the first 3 are very similar. However, I cannot do top-level
from sage.libs.gap.element import GapElement_FiniteField in each of them, only in element_ntl_gf2e.pyx it does not
lead to an obscure runtime error (Cannot import RDF) from libgap.
Replying to Matthias Köppe:
Another approach: #34804 Deprecate
sage.interfacesis_...Elementfunctions
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.
Replying to Dima Pasechnik:
Replying to Matthias Köppe:
Another approach: #34804 Deprecate
sage.interfacesis_...ElementfunctionsOK, this is interesting. So this would mean unconditionally importing
GapElementfrom
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.pyxlook 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
Yes, looking good.
review?
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.
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?
Replying to Dima Pasechnik:
Why don't we instead go the way we went with
GapElementhere, by addingsage.libs.abc, etc?
That's also fine
Replying to Matthias Köppe:
Replying to Dima Pasechnik:
Why don't we instead go the way we went with
GapElementhere, by addingsage.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.
It's kind of why I suggested to use try except ... () in comment:30.
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.
The ticket is introducing a modularization regression by inserting this new module-level import.
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.
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.
Replying to Matthias Köppe:
There are no unprotected module-level imports from
sage.libs.gapin 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.
Branch pushed to git repo; I updated commit sha1. New commits:
6e5ea79 | move import of libgap into a function |
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."
Reviewer: Matthias Koeppe
LGTM, thanks.
Changed branch from u/dimpase/rings/FFEs_to_accept_libgap to 6e5ea79