Better coercion to ZZ for libGAP integers, modular integers

[kedlaya]

This PR implements some more sensible conversions for libGAP integers and modular integers.

For integers, we fix a TODO to convert long libGAP integers by directly accessing their underlying GMP integers. The current implementation instead uses a decimal string representation as an intermediate step.

For modular integers, we implement a lift method; this allows the .sage method to use a more direct coercion into the appropriate ring.

[kedlaya]

Is this duplicating changes in gappy by any chance?

[dimpase]

AFAIK, gappy didn't go there, it's an unfinished (sadly) attempt to spin the Cython interface to (lib)gap into a separate Python package.

[dimpase]

Oops, no, my memory served me wrong, this part indeed was fixed in gappy, more precisely, in gappy+reworked Sage inteface to gappy, see https://github.com/sagemath/sagetrac-mirror/tree/u/dimpase/gappy-without-wrappers (look at the diff of element.pyx and see the chunk with "waste" comment removed)

[tscrim]

However, shouldn't we just fix this within our own code (independently of gappy)?

[dimpase]

well, it would be nice, but this PR doesn't work - exactly at this place, it seems, see CI output
(nowadays you can just click on https://github.com/sagemath/sage/pull/37580/files and scroll down)

[kedlaya]

Oops, it looks like I pushed the wrong version of the code originally. This is now fixed and the checks seem to be passing now.

[dimpase]

do we really need this else? As far as I understand from Int GAP_SizeInt(Obj obj) you get size==0, the only 3rd possible case, if self.value==0. In any event, there seem to be nothing "wrong" with getting 0 here.

@fingolfin - is this right?

Note that if I call GAP_SizeInt() on a non-integer, I'll get

Error, GAP_SizeInt: <obj> must be an integer (not a list (string))

As far as I see, GAP's Ints are created by (GAP_)MakeObjInt(limbs,size), with limbs an array of unsigned ints, and size being >0 for getting a positive Int, <0 for getting negatve Int, and 0 for getting 0.

As far as I understand, c_size should be 1 if size==0.

[kedlaya]

In fact the else should never be triggered: if self.value == 0 then self.is_C_int() should return True, so we don't reach this if in the first place.

[fingolfin]

@dimpase yes that's right (and documented like that).

Of course @kedlaya is also right -- but it seems odd to fall back to string representation then: if you are at this point, something major is wrong...

[dimpase]

@fingolfin - oops, I should have looked at GAP's src/integer.h

(or is there any other source for docs?)

[fingolfin]

You could also do this:

Suggested change

	sage: n = libgap.eval('One(ZmodnZ(123)) * 13')
	sage: n = libgap.eval('ZmodnZObj(13, 123))')

or, if libgap makes it easy to directly call GAP functions with small int arguments, then something like this:

Suggested change

	sage: n = libgap.eval('One(ZmodnZ(123)) * 13')
	sage: n = libgap.ZmodnZObj(13, 123)

[kedlaya]

Looking at this file some more, I see various GAP commands being surrounded by GAP_Enter() (or sometimes sig_GAP_Enter()) and GAP_Leave. Does anyone know when these are needed? Is it safe to omit them when executing commands that access GAP objects without creating or modifying them?

[github-actions]

Documentation preview for this PR (built with commit 2bcbfd6; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[vbraun]

GAP_Enter is for marking the stack bottom. Gap's garbage collector will scan through the stack and look for pointers that look like gap memory bags to keep them alive (accepting possibly false positives). If the stack bottom is wrong then it might erroneously garbage collect gap memory bags.

Not all gap commands can cause garbage collection to run (roughly the C macros don't, but thats not a 100% reliable rule). If in doubt its better to add the GAP_Enter (which is super-fast, just writes one pointer) than to cause hard-to-debug bugs where an alive object can be erroneously garbage collected.

[kedlaya]

Fair enough, I added GAP_Enter and GAP_Leave calls to the key method.

[fchapoton]

looks good to me