Closed fingolfin closed 3 years ago
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
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.
Dear all,
Thanks for doing all the recent changes.
I just had a look at that test file; I did the calculation manually, and the output seems fine to me:
[image: image.png]
I did this one the following (Mac OS): [image: image.png]
I have no idea why your automatic test reports that the output is not as expected (?)
Best wishes, Heiko
On Thu, 8 Apr 2021 at 06:57, Max Horn @.***> wrote:
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.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/gap-packages/corelg/pull/10#issuecomment-815258098, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADNA76QRTMQWEMNNN3XG2H3THTBMNANCNFSM42NQR4RA .
-- Dr Heiko Dietrich Associate Professor Director of Postgraduate Studies School of Mathematics, Monash University users.monash.edu/~heikod
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?
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
).
Dear Alexander, (Cc: dear Willem)
The pictures just showed that I was not able to reproduce the reported behaviour. I've copied+pasted the relevant piece of code but then got the expected outputs (?) Moreover I haven't really made any changes to the package, so I cannot understand why this test suddenly fails. (I understand we simply compute the ID and compare with the printed output. I get the expected output.)
Willem, does it work on your machine?
I will try with your suggestion (higher assertion level) later today.
Best wishes Heiko
Dr Heiko Dietrich Associate Professor Director of Postgraduate Studies School of Mathematics, Monash University users.monash.edu/~heikod
On Thu, 22 Apr 2021, 07:01 Alexander Konovalov, @.***> wrote:
P.S. @heikodietrich https://github.com/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).
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gap-packages/corelg/pull/10#issuecomment-824354918, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADNA76UWGTMVIIXTLJEJVBDTJ44KFANCNFSM42NQR4RA .
Hello again,
Here is the output I had included as screenshots; it's as expected and I cannot reproduce the error.
I've just copied+pasted the relevant test code; I don't know how to run it as an 'official' test... sorry!
Best wishes, Heiko
mopspower:/Volumes/daten/heiko$ gap11 ┌───────┐ GAP 4.11.0 of 29-Feb-2020 │ GAP │ https://www.gap-system.org └───────┘ Architecture: x86_64-apple-darwin19.6.0-default64-kv7 Configuration: gmp 6.2.0, GASMAN Loaded workspace: /Volumes/daten/gap-4.11.0/bin/ws.gap Packages: AClib 1.3.2, Alnuth 3.1.2, AtlasRep 2.1.0, AutoDoc 2019.09.04, AutPGrp 1.10.2, CRISP 1.4.5, Cryst 4.1.23, CrystCat 1.1.9, CTblLib 1.2.2, FactInt 1.6.3, FGA 1.4.0, GAPDoc 1.6.3, IRREDSOL 1.4, LAGUNA 3.9.3, Polenta 1.3.9, Polycyclic 2.15.1, PrimGrp 3.4.0, RadiRoot 2.8, ResClasses 4.7.2, SmallGrp 1.4.1, Sophus 1.24, SpinSym 1.5.2, TomLib 1.2.9, TransGrp 2.0.5, utils 0.69 Try '??help' for help. See also '?copyright', '?cite' and '?authors' gap> LoadPackage("corelg"); | QuaGroup 1.8.2 |
---|
----------- A package for dealing with quantized enveloping algebras | | Willem de Graaf | @.***
───────────────────────────────────────────────────────────────────────────── Loading SLA 1.5.3 (Computing with simple Lie algebras) by Willem Adriaan de Graaf (http://www.science.unitn.it/~degraaf). maintained by: Willem Adriaan de Graaf (http://www.science.unitn.it/~degraaf) and The GAP Team @.). Homepage: https://gap-packages.github.io/sla/ Report issues at https://github.com/gap-packages/sla/issues ───────────────────────────────────────────────────────────────────────────── ───────────────────────────────────────────────────────────────────────────── Loading CoReLG 1.54dev (Computing with real Lie algebras) by Heiko Dietrich (http://users.monash.edu.au/~heikod/), Paolo Faccin @.), and Willem de Graaf (https://www.science.unitn.it/~degraaf). Homepage: https://gap-packages.github.io/corelg/ Report issues at https://github.com/gap-packages/corelg/issues ───────────────────────────────────────────────────────────────────────────── true 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) su(2)+sp(1,2) so(8,1) so(9) sl(2,R)+G2c gap>
On Thu, 22 Apr 2021 at 06:59, Alexander Konovalov @.***> wrote:
hi @heikodietrich https://github.com/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?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gap-packages/corelg/pull/10#issuecomment-824353773, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADNA76RP2W7XQUV4PRFTEMTTJ44BTANCNFSM42NQR4RA .
-- Dr Heiko Dietrich Associate Professor Director of Postgraduate Studies School of Mathematics, Monash University users.monash.edu/~heikod
@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).
Hi Alex,
With 4.10 I get the same: mopspower:/Volumes/daten/gap-4.10.0/bin$ ./gap.sh ┌───────┐ GAP 4.10.0 of 01-Nov-2018 │ GAP │ https://www.gap-system.org └───────┘ Architecture: x86_64-apple-darwin17.4.0-default64 Configuration: gmp 6.2.0 Loading the library and packages ... Packages: AClib 1.3.1, Alnuth 3.1.0, AtlasRep 1.5.1, AutoDoc 2018.09.20, AutPGrp 1.10, CRISP 1.4.4, Cryst 4.1.18, CrystCat 1.1.8, CTblLib 1.2.2, FactInt 1.6.2, FGA 1.4.0, GAPDoc 1.6.2, IRREDSOL 1.4, LAGUNA 3.9.0, Polenta 1.3.8, Polycyclic 2.14, PrimGrp 3.3.2, RadiRoot 2.8, ResClasses 4.7.1, SmallGrp 1.3, Sophus 1.24, SpinSym 1.5, TomLib 1.2.7, TransGrp 2.0.4, utils 0.59 Try '??help' for help. See also '?copyright', '?cite' and '?authors' gap> LoadPackage("corelg"); | QuaGroup |
---|
----------- A package for dealing with quantized enveloping algebras | | Willem de Graaf | @.***
─────────────────────────────────────────────────────────────────────────────────────────────────────────── Loading SLA 1.5.2 (Computing with simple Lie algebras) by Willem Adriaan de Graaf (http://www.science.unitn.it/~degraaf). Homepage: https://gap-packages.github.io/sla/ ─────────────────────────────────────────────────────────────────────────────────────────────────────────── ─────────────────────────────────────────────────────────────────────────────────────────────────────────── Loading CoReLG 1.54 (Computing with real Lie algebras) by Heiko Dietrich (http://users.monash.edu.au/~heikod/), Paolo Faccin @.***), and Willem de Graaf (http://www.science.unitn.it/~degraaf). Homepage: https://gap-packages.github.io/corelg/ ─────────────────────────────────────────────────────────────────────────────────────────────────────────── true 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) su(2)+sp(1,2) so(8,1) so(9) sl(2,R)+G2c gap>
Moreover, I am confused about the original error; the following is from the original report:
########> Diff in /home/runner/work/corelg/corelg/gaproot/pkg/corelg/tst/corelg02.tst:74
for K in r.subalgs do Print( NameRealForm(K), "\n" ); od;
su(1,2)+su(3) su(2)+sp(1,2) so(8,1) so(9) sl(2,R)+G2c
su(1,2)+su(3) Error, Assertion failure
What does the "But found:" refer to, the whole list, or only one entry? What is listed is part of the expected list (first entry)...
Cheers, Heiko
On Thu, 22 Apr 2021 at 08:28, Alexander Konovalov @.***> wrote:
@heikodietrich https://github.com/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 drop supporting them).
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gap-packages/corelg/pull/10#issuecomment-824403630, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADNA76XWXCUJZ5CEIA7NZI3TJ5GQXANCNFSM42NQR4RA .
-- Dr Heiko Dietrich Associate Professor Director of Postgraduate Studies School of Mathematics, Monash University users.monash.edu/~heikod
@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>
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.
@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
This seems to me to be a problem in Factors( DefaultRing( [ r ] ), r ) at /Volumes/daten/gap-4.10.0/lib/ ring.gi:1019 called from
Please see below.
Cheers, Heiko
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 /Volumes/daten/gap-4.10.0/lib/ cyclotom.gi:1943 called from Factors( DefaultRing( [ r ] ), r ) at /Volumes/daten/gap-4.10.0/lib/ ring.gi:1019 called from Factors( f ) at /Volumes/daten/gap-4.10.0/pkg/corelg/gap/carrierrtsys.gi:277 called from RootsystemOfCartanSubalgebra( L, h ) at /Volumes/daten/gap-4.10.0/pkg/corelg/gap/realforms.gi:2075 called from VoganDiagram( L0 ) at /Volumes/daten/gap-4.10.0/pkg/corelg/gap/ realforms.gi:3941 called from NameRealForm( K ) at stdin:8 called from ... at stdin:8 you may 'return;' brk> pol; x_1^5+(-2E(4))x_1^4+8x_1^3+(-16E(4))x_1^2+16x_1+(-32E(4)) brk> factors; [ x_1, x_1, x_1+(-2E(4)), x_1+(-2E(4)), x_1+(-2E(4)), x_1+2E(4), x_1+2E(4) ] brk> factors[1]factors[2]factors[3]factors[4]factors[5]factors[6]factors[7]; x_1^7+(-2E(4))x_1^6+8x_1^5+(-16E(4))x_1^4+16x_1^3+(-32E(4))x_1^2 brk>
brk> f; x_1^7+(-2E(4))x_1^6+8x_1^5+(-16E(4))x_1^4+16x_1^3+(-32E(4))x_1^2 brk> Factors(f); Error, Assertion failure in Assert( 2, Product( factors ) = pol ); at /Volumes/daten/gap-4.10.0/lib/ cyclotom.gi:1943 called from Factors( DefaultRing( [ r ] ), r ) at /Volumes/daten/gap-4.10.0/lib/ ring.gi:1019 called from Assert( 2, Product( factors ) = pol ); at /Volumes/daten/gap-4.10.0/lib/ cyclotom.gi:1943 called from Factors( DefaultRing( [ r ] ), r ) at /Volumes/daten/gap-4.10.0/lib/ ring.gi:1019 called from Factors( f ) at /Volumes/daten/gap-4.10.0/pkg/corelg/gap/carrierrtsys.gi:277 called from RootsystemOfCartanSubalgebra( L, h ) at /Volumes/daten/gap-4.10.0/pkg/corelg/gap/realforms.gi:2075 called from ... at errin:5 you may 'return;' brk_2>
On Thu, 22 Apr 2021 at 08:34, Alexander Konovalov @.***> wrote:
@heikodietrich https://github.com/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>
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gap-packages/corelg/pull/10#issuecomment-824406294, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADNA76UKDHHH5AI4J53QVN3TJ5HGVANCNFSM42NQR4RA .
-- Dr Heiko Dietrich Associate Professor Director of Postgraduate Studies School of Mathematics, Monash University users.monash.edu/~heikod
Weird. I can't reproduce it with assertion level 1. Well, maybe require GAP 4.11?
Thanks -- yes, indeed, the degree of the product of the factors is not even correct.
On Thu, 22 Apr 2021 at 08:36, Alexander Konovalov @.***> wrote:
@fingolfin https://github.com/fingolfin @heikodietrich https://github.com/heikodietrich furthermore, look at what caused assertion failure:
brk> pol; x_1^5+(-2E(4))x_1^4+8x_1^3+(-16E(4))x_1^2+16x_1+(-32E(4)) brk> factors; [ x_1, x_1, x_1+(-2E(4)), x_1+(-2E(4)), x_1+(-2E(4)), x_1+2E(4), x_1+2E(4) ] brk> Product(factors); x_1^7+(-2E(4))x_1^6+8x_1^5+(-16E(4))x_1^4+16x_1^3+(-32E(4))x_1^2
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gap-packages/corelg/pull/10#issuecomment-824406960, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADNA76SKY4M3KG27M3XKNQLTJ5HNLANCNFSM42NQR4RA .
-- Dr Heiko Dietrich Associate Professor Director of Postgraduate Studies School of Mathematics, Monash University users.monash.edu/~heikod
Oh, and to run the tests "the official way", you can do gap tst/testall.g in the corelg directory. Great, thanks Max! I hope I will remember for next time...
Cheers, Heiko
On Thu, 22 Apr 2021 at 08:35, Max Horn @.***> wrote:
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.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gap-packages/corelg/pull/10#issuecomment-824406621, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADNA76QHJX5EDLOSK4IOJLDTJ5HJ7ANCNFSM42NQR4RA .
-- Dr Heiko Dietrich Associate Professor Director of Postgraduate Studies School of Mathematics, Monash University users.monash.edu/~heikod
@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 https://github.com/gap-system/gap/pull/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.
But the test fails for a reason: the product of the factors has degree 7, but the polynomial should have degree 5 (?)
Strange...
But as far as I understand it, this is only triggered by corelg -- not a bug in corelg. I don't think I can really help with the bug in cyclotomic :-O
On Thu, 22 Apr 2021 at 08:43, Max Horn @.***> wrote:
@alex-konovalov https://github.com/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 https://github.com/gap-system/gap/pull/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.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gap-packages/corelg/pull/10#issuecomment-824411972, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADNA76U3MXSJFPRPEEUQTTDTJ5IJZANCNFSM42NQR4RA .
-- Dr Heiko Dietrich Associate Professor Director of Postgraduate Studies School of Mathematics, Monash University users.monash.edu/~heikod
@fingolfin ah, ok, thanks for the link - I was assuming factorisation went wrong, not the assertion.
@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
On the upside, this "discovery" motivated me to file https://github.com/gap-actions/setup-gap/issues/24
All good now :-)
This is necessary as Travis CI started to require payment for its services.