Skip to content

Conversation

@antonio-rojas
Copy link
Contributor

GAP 4.15 causes a few test failures caused mostly by different ordering of group characters and different elements returned for an_element in group algebras, make test pass with both old and new versions.

@github-actions
Copy link

github-actions bot commented Sep 29, 2025

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

@collares
Copy link
Contributor

Not sure if you addressed this already, but it seems that some tests are now really slow:

sage -t --long --warn-long 30.0 --random-seed=48470199707196632862320241923438074047 /nix/store/lcqvpnmfb5p7j07wzxialg4315qv2k1p-sage-src-10.6/src/sage/groups/libgap_morphism.py
**********************************************************************
File "/nix/store/lcqvpnmfb5p7j07wzxialg4315qv2k1p-sage-src-10.6/src/sage/groups/libgap_morphism.py", line 452, in sage.groups.libgap_morphism.GroupMorphism_libgap._call_
Warning: slow doctest:
    f = O.hom([r*x*r_ for x in O.gens()])  # long time (19s on sage.math, 2011)
Test ran for 521.46s cpu, 524.36s wall
Check ran for 0.00s cpu, 0.00s wall
    [209 tests, 524.92s wall]

@antonio-rojas antonio-rojas marked this pull request as draft September 30, 2025 05:20
@antonio-rojas
Copy link
Contributor Author

Not sure if you addressed this already, but it seems that some tests are now really slow:

sage -t --long --warn-long 30.0 --random-seed=48470199707196632862320241923438074047 /nix/store/lcqvpnmfb5p7j07wzxialg4315qv2k1p-sage-src-10.6/src/sage/groups/libgap_morphism.py
**********************************************************************
File "/nix/store/lcqvpnmfb5p7j07wzxialg4315qv2k1p-sage-src-10.6/src/sage/groups/libgap_morphism.py", line 452, in sage.groups.libgap_morphism.GroupMorphism_libgap._call_
Warning: slow doctest:
    f = O.hom([r*x*r_ for x in O.gens()])  # long time (19s on sage.math, 2011)
Test ran for 521.46s cpu, 524.36s wall
Check ran for 0.00s cpu, 0.00s wall
    [209 tests, 524.92s wall]

By "now" do you mean with GAP 4.15? This PR only changes the expected output of some tests

@collares
Copy link
Contributor

collares commented Sep 30, 2025

Yes, sorry, I should have been more clear. The test I mentioned seems to be a lot slower on GAP 4.15 than it was on GAP 4.14. More specifically, testing src/sage/groups/libgap_morphism.py goes from 3 seconds on GAP 4.14 to at least 460 seconds (minimum of three runs) on GAP 4.15, and it doesn't seem to be seed-specific.

@antonio-rojas
Copy link
Contributor Author

Yes, sorry, I should have been more clear. The test I mentioned seems to be a lot slower on GAP 4.15 than it was on GAP 4.14. More specifically, testing src/sage/groups/libgap_morphism.py goes from 3 seconds on GAP 4.14 to at least 460 seconds (minimum of three runs) on GAP 4.15, and it doesn't seem to be seed-specific.

Indeed. I have reported it upstream at gap-system/gap#6135

@antonio-rojas
Copy link
Contributor Author

Tests are passing now with GAP 4.14 and 4.15 modulo the upstream bugs gap-system/gap#6133 and gap-system/gap#6135, ready for review

@antonio-rojas antonio-rojas marked this pull request as ready for review October 3, 2025 16:26
@cxzhong
Copy link
Contributor

cxzhong commented Oct 18, 2025

the GAP4.15.1 is out.

@antonio-rojas
Copy link
Contributor Author

All tests pass now with unpatched 4.15.1

@cxzhong
Copy link
Contributor

cxzhong commented Oct 19, 2025

All tests pass now with unpatched 4.15.1

Thank you. I want to know just using random is ok?

@antonio-rojas
Copy link
Contributor Author

All tests pass now with unpatched 4.15.1

Thank you. I want to know just using random is ok?

For the examples in the doc source files it should be, those are already widely tested in the code itself.

@cxzhong
Copy link
Contributor

cxzhong commented Oct 19, 2025

All tests pass now with unpatched 4.15.1

Thank you. I want to know just using random is ok?

For the examples in the doc source files it should be, those are already widely tested in the code itself.

Thank you. I will set positive review.

@orlitzky
Copy link
Contributor

Adding # random doesn't really make the tests pass with both versions, it just ignores any failures short of a segfault/timeout.

We can check the GAP version with libgap.GAPInfo["Version"] at runtime. Adding # needs gap414 and # needs gap415 would take only a few lines, and would leave behind useful examples that are actually tested. I've done this with # needs mpmath13 in #41014 for example and it was painless.

@antonio-rojas
Copy link
Contributor Author

Adding # random doesn't really make the tests pass with both versions, it just ignores any failures short of a segfault/timeout.

We can check the GAP version with libgap.GAPInfo["Version"] at runtime. Adding # needs gap414 and # needs gap415 would take only a few lines, and would leave behind useful examples that are actually tested. I've done this with # needs mpmath13 in #41014 for example and it was painless.

I'm aware of that, but that is not something that would look good in an examples block in the middle of an html documentation page whose main purpose is to teach users how sage works. I actually question the need to test documentation source files at all, which just repreat the tests that have already been run in the main code up to 10 times (once per language).

@orlitzky
Copy link
Contributor

I'm aware of that, but that is not something that would look good in an examples block in the middle of an html documentation page whose main purpose is to teach users how sage works. I actually question the need to test documentation source files at all, which just repreat the tests that have already been run in the main code up to 10 times (once per language).

I don't think re-testing the code was ever the intention. It helps keep the documentation from falling out of date. Right now,

sage: G.character_table() # random
[ 1  1  1  1  1]
[ 1 -1 -1  1  1]
[ 1 -1  1 -1  1]
[ 1  1 -1 -1  1]
[ 2  0  0  0 -2]

looks reasonable because we know what it does in both recent versions of GAP, and "random" means "ignore minor (mathematically unimportant) differences." But this test will continue to pass if someone deletes the character_table() method two years from now. In situations like those it's nice to get a heads up.

@antonio-rojas
Copy link
Contributor Author

looks reasonable because we know what it does in both recent versions of GAP, and "random" means "ignore minor (mathematically unimportant) differences." But this test will continue to pass if someone deletes the character_table() method two years from now. In situations like those it's nice to get a heads up.

Well, if someone deletes the method this test would throw an error. The worst that could happen is that some output format changes are not reflected in the documentation. I see this as any other instance of manually maintained docs getting out of date. Ideally there would be a way to "link" an examples block from a py file in the rst file so the changes would automatically be picked up.

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 26, 2025
sagemathgh-40919: Make tests pass with GAP 4.15
    
GAP 4.15 causes a few test failures caused mostly by different ordering
of group characters and different elements returned for `an_element` in
group algebras, make test pass with both old and new versions.
    
URL: sagemath#40919
Reported by: Antonio Rojas
Reviewer(s):
@vbraun vbraun merged commit 54d4ddb into sagemath:develop Oct 27, 2025
21 of 24 checks passed
@antonio-rojas antonio-rojas deleted the gap-4.15 branch October 28, 2025 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants