Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Switch CI to use GitHub Actions #10

Merged
merged 3 commits into from
Apr 21, 2021
Merged

Switch CI to use GitHub Actions #10

merged 3 commits into from
Apr 21, 2021

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Apr 5, 2021 •
edited
Loading

This is necessary as Travis CI started to require payment for its services.

@codecov
Copy link

codecov bot commented Apr 5, 2021 •
edited
Loading

Codecov Report

Merging #10 (c1a0a80) into master (f6b3bf9) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #10   +/-   ##
=======================================
  Coverage   92.01%   92.01%           
=======================================
  Files          16       16           
  Lines      180360   180360           
=======================================
  Hits       165956   165956           
  Misses      14404    14404

@fingolfin
Copy link
Member Author

There is a CI failures with GAP 4.9 and 4.10, and I think we had a workaround before; the second commit in this PR tries to fix that.

But still, there is a test error, namely this one:

########> Diff in /home/runner/work/corelg/corelg/gaproot/pkg/corelg/tst/corelg02.tst:74
# Input is:
for K in r.subalgs do Print( NameRealForm(K), "\n" ); od;
# Expected output:
su(1,2)+su(3)
su(2)+sp(1,2)
so(8,1)
so(9)
sl(2,R)+G2c
# But found:
su(1,2)+su(3)
Error, Assertion failure

I didn't have time to digger deeper yet, help is welcome.

@heikodietrich
Copy link
Collaborator

heikodietrich commented Apr 8, 2021 via email

@olexandr-konovalov
Copy link
Member

hi @heikodietrich - GitHub blocks images when you reply to a comment by email. If they are essential, could you please login to GitHub and edit your comment above to add images, or copy and paste the text from the terminal as a plain text?

@olexandr-konovalov
Copy link
Member

P.S. @heikodietrich You may hopefully be able to reproduce it locally, if you increase assertion level above the default one. Tests use a higher level (triggered by a call to START_TEST).

@heikodietrich
Copy link
Collaborator

heikodietrich commented Apr 21, 2021 via email

@heikodietrich
Copy link
Collaborator

heikodietrich commented Apr 21, 2021 via email

@olexandr-konovalov
Copy link
Member

olexandr-konovalov commented Apr 21, 2021 •
edited
Loading

@heikodietrich the error occurs in branches corresponding to GAP 4.10 and GAP 4.9 - you will not see it in GAP 4.11 or master. Need to check with older GAP versions (or stop supporting them).

@heikodietrich
Copy link
Collaborator

heikodietrich commented Apr 21, 2021 via email

@olexandr-konovalov
Copy link
Member

@heikodietrich managed to reproduce it with SetAssertionLevel(2):

gap> SetAssertionLevel(2);
gap> r := MaximalReductiveSubalgebras("F",4,3);;           
gap> NameRealForm(r.liealg);                               
"F4(-20)"
gap> for K in r.subalgs do Print( NameRealForm(K),"\n"); od;
su(1,2)+su(3)
Error, Assertion failure in
  Assert( 2, Product( factors ) = pol 
 ); at /Users/alexk/gap/gap-4.10.2/lib/cyclotom.gi:1943 called from 
Factors( DefaultRing( [ r ] ), r 
 ) at /Users/alexk/gap/gap-4.10.2/lib/ring.gi:1019 called from
Factors( f 
 ) at /Users/alexk/Library/Preferences/GAP/pkg/corelg/gap/carrierrtsys.gi:277 called from
RootsystemOfCartanSubalgebra( L, h 
 ) at /Users/alexk/Library/Preferences/GAP/pkg/corelg/gap/realforms.gi:2075 called from
VoganDiagram( L0 
 ) at /Users/alexk/Library/Preferences/GAP/pkg/corelg/gap/realforms.gi:3938 called from
NameRealForm( K ) at *stdin*:13 called from
...  at *stdin*:13
you may 'return;'
brk>

@fingolfin
Copy link
Member Author

Note that tests run with SetAssertionLevel(2);. And the assertion problem is not new; I worked around it before, see this excerpt from tst/testall.g:

# There is a bug in GAP <= 4.10 where factorizing certain polynomials over the
# cyclotomics triggers an incorrect assertion if the assertion level is 2 or
# higher. Unfortunately, the manual tests of corelg trigger the problematic
# case, and START_TEST sets the assertion level to 2 by default. To workaround
# that, we modify START_TEST to only set the assertion level to 1.
if not CompareVersionNumbers(GAPInfo.Version, "4.11") then
    original_START_TEST := START_TEST;
    START_TEST := function( name )
        original_START_TEST( name );
        SetAssertionLevel( 1 );
    end;
fi;

What I don't understand right now is why this workaround doesn't work.

Oh, and to run the tests "the official way", you can do gap tst/testall.g in the corelg directory.

@olexandr-konovalov
Copy link
Member

@fingolfin @heikodietrich furthermore, look at what caused assertion failure:

brk> pol; 
x_1^5+(-2*E(4))*x_1^4+8*x_1^3+(-16*E(4))*x_1^2+16*x_1+(-32*E(4))
brk> factors;
[ x_1, x_1, x_1+(-2*E(4)), x_1+(-2*E(4)), x_1+(-2*E(4)), x_1+2*E(4), 
  x_1+2*E(4) ]
brk> Product(factors);
x_1^7+(-2*E(4))*x_1^6+8*x_1^5+(-16*E(4))*x_1^4+16*x_1^3+(-32*E(4))*x_1^2

@heikodietrich
Copy link
Collaborator

heikodietrich commented Apr 21, 2021 via email

@olexandr-konovalov
Copy link
Member

Weird. I can't reproduce it with assertion level 1. Well, maybe require GAP 4.11?

@heikodietrich
Copy link
Collaborator

heikodietrich commented Apr 21, 2021 via email

@heikodietrich
Copy link
Collaborator

heikodietrich commented Apr 21, 2021 via email

@fingolfin
Copy link
Member Author

@alex-konovalov no, just blindly requiring 4.11 is not a solution, first we need to understand the problem

Please read the excerpt from tst/testall.g I posted above. It explains the situation. If you want details, take a look at gap-system/gap#3850 -- the factorization code is correct, only the assertion check is wrong.

The only weird part is why my workaround does not take effect. But I'll figure it out; I just had (and have) other things on my plate, too, and this is a low priority right now, after all, this is just internal tooling / infrastructure, nothing that affects corelg as such or would require a release of it or whatever.

@heikodietrich
Copy link
Collaborator

heikodietrich commented Apr 21, 2021 via email

@olexandr-konovalov
Copy link
Member

@fingolfin ah, ok, thanks for the link - I was assuming factorisation went wrong, not the assertion.

@fingolfin
Copy link
Member Author

@heikodietrich again, the link I posted explains this (hint: what you see is misleading: the original polynomial did have degree 7, the factorization is correct, just the assertion is not).

OK, I understand now what's going on: we are not testing with an actual GAP 4.10 release, but rather with the stable-4.10 branch. And in there, GAP has version 4.dev... so my version based check does not work. Easy enough to resolve

@fingolfin
Copy link
Member Author

On the upside, this "discovery" motivated me to file gap-actions/setup-gap#24

@fingolfin fingolfin merged commit 84194f5 into master Apr 21, 2021
@fingolfin fingolfin deleted the mh/travis-to-gh-actions branch April 21, 2021 23:26
@fingolfin
Copy link
Member Author

All good now :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fingolfin @heikodietrich @olexandr-konovalov