sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.
https://www.sagemath.org
Other
1.08k stars 394 forks source link

gap: support gap 4.13 in sagelib (does NOT touch sage-the-distro) #37884

Open tornaria opened 2 weeks ago

tornaria commented 2 weeks ago

In gap 4.13 there are some improvements, e.g. converting fp groups to permutation groups, computing abelianization of fp groups, which lead to different generators.

This PR fixes doctests so they pass using gap 4.13.

I don't see a way to have this work with gap 4.12.2 at the same time, so this is not in principle ready to merge.

:memo: Checklist

:hourglass: Dependencies

37883: don't use deprecated LaTeX() gap function.

github-actions[bot] commented 2 weeks ago

Documentation preview for this PR (built with commit 7a30869c20801b585344ab246df5861c5f22393f; changes) is ready! :tada: This preview will update shortly after each push to this PR.

tornaria commented 2 weeks ago

The failures here are real:

----------------------------------------------------------------------
sage -t --random-seed=130925441006892428060280565126983766193 src/sage/categories/simplicial_sets.py  # 7 doctests failed
sage -t --random-seed=130925441006892428060280565126983766193 src/sage/groups/finitely_presented.py  # 5 doctests failed
sage -t --random-seed=130925441006892428060280565126983766193 src/sage/groups/perm_gps/permgroup_named.py  # 3 doctests failed
----------------------------------------------------------------------

and the reason why this PR can't be merged if gap is not updated to 4.13.

But really, either output (the one with gap 4.12 or the one with 4.13) is just fine. Hence, updating gap to 4.13 in sage-the-distro and apply this PR at the same time, is a pain for using system gap (either we restrict the version of gap that is allowed from system, or we end up with failing doctests on some systems).

We really need to come up with a system to be able to support different versions of dependencies in doctests... at this time we have a similar problem with the difference between singular 4.3 and 4.4 (which we worked around with an ugly hack of patching the output of singular when running via doctests!).

tscrim commented 2 weeks ago

We had specific tags for Python 2 and 3 differences. Since there are only a few differences (some of which are really not easy to rectify by changing the doctests), one option might be to introduce such tags for GAP versions. Or we just tell people that some tests might not pass for trivial reasons if you do not have a sufficiently updated version of GAP...

tornaria commented 2 weeks ago

We had specific tags for Python 2 and 3 differences. Since there are only a few differences (some of which are really not easy to rectify by changing the doctests), one option might be to introduce such tags for GAP versions. Or we just tell people that some tests might not pass for trivial reasons if you do not have a sufficiently updated version of GAP...

Here's one idea to think about:

A quick example with one test (very crude first try):

In src/sage/algebras/fusion_rings/fusion_double.py we have

        sage: b13^2 # long time (4s), gap-4.12.2
        b0 + b2 + b4 + b15 + b16 + b17 + b18 + b24 + b26 + b27

The traslation file for gap has something like:

#: src/sage/algebras/fusion_rings/fusion_double.py:134
msgid b0 + b2 + b4 + b15 + b16 + b17 + b18 + b24 + b26 + b27
msgstr[4.13.0] b0 + b3 + b4

I'm not sure if the doctest label has to include the gap version or even the program name (maybe just have a common tag to mark the test to be applied this mechanism).

tornaria commented 2 weeks ago

Here's another (maybe simpler?) idea. See src/sage/arith/long.pxd where we have this doctest:

        sage: a^(2**258)
        Traceback (most recent call last):
        ...
        OverflowError: exponent must be at most 2147483647           # 32-bit
        OverflowError: exponent must be at most 9223372036854775807  # 64-bit

so the output is labeled here... If we could have labels e.g "gap < 4.13" and "gap >= 4.13" or something like this... Would that we workable?

tscrim commented 4 days ago

Sorry for loosing track of this. Yes, I think having something as a tag on the output with, e.g., # gap < 4.13 is a good solution. It is clear to the reader and to implement in the tester. This is what I had in mind.