New tests for group constructors and some fixes #1053

fingolfin · 2017-01-07T16:24:12Z

add a bunch of tests for group constructors in the grp directory
add tests for for various matrix groups that verify the given invariant forms
fix GO(1,4,5) (triggers an error on master)
add invariant forms for Sp(2*n, Integers mod 2^k)
remove cache limit from perfect groups library, to increase speed
remove some invalid (and unusable) constructor variants
tweak error messages in some group constructors

codecov-io · 2017-01-07T17:08:09Z

Current coverage is 53.97% (diff: 100%)

No coverage report found for master at 34e30fb.

Powered by Codecov. Last update 34e30fb...3a85e11

ChrisJefferson · 2017-01-08T12:37:24Z

These all look good and uncontroversial. Will merge unless someone says something in the next couple of days.

It is annoying it looks like coverage got worse, as tst files aren't marked as covered I think.

fingolfin · 2017-01-08T13:50:28Z

Ahhhh, is that why? hmmm, so how are the total lines reported to / computed by codecov? I would havethought it only counts source files in lib, src, ... as shown on their website...

fingolfin · 2017-01-08T15:01:02Z

I think the tes files are not the reason for this change. Rather, what really happened is that codecov ignores files which have neither hits nor misses. With this PR, it soundly "learned" about some additional source files (several perf*.grp files) which are not covered.

Also updated some comments explaining why certain tests are excluded from testinstall.

fingolfin · 2017-01-08T18:04:33Z

Something weird is going on: I added a brutal test which invokes PerfectGroup for every legal input. So it should in theory show 100% coverage for all the perfNN.grp files. Yet it doesn't, it even reports 0% coverage for some of them. Huh?

@ChrisJefferson any idea what might be going wrong here?

These error message (and also those for the classical groups) seem to vary quite a bit. We may want to unify them at some point.

For some strange reasons, many group constructor wrapper allow the user to pass one more argument than documented. I could not find any methods supporting these extra arguments. Looking at the old repository, these extra calls already appear in the first GAP4 version of this code, added by Frank Celler 1997-01-15. So I think it's either a copy&paste mistake, or a remnant of GAP3.

ChrisJefferson · 2017-01-08T18:42:21Z

Good question -- not sure.

If I manually run PerfectGroup(7800), then I get coverage of the first part of perf2.grp, so it's some combination of the tests being run, and the information going to testcov I think. For some reason, I'm having trouble checking out your branch for some reason to try it out.

ChrisJefferson · 2017-01-08T18:50:14Z

Nope, it is my fault, something weird is going on (your tests lead to weirdly corrupted profiling output. I think it's because the files keep getting re-read. Investigating).

ChrisJefferson · 2017-01-08T19:05:40Z

Turns out profiling breaks with re-reading of files! Will looking into fixing that.

Independently, the Perfect Groups library only keeps a cache of 50 groups (if I'm reading it right), perf.grp, line 31. If I push that to 1000, then perf.grp goes from 2.7 sec to 0.5 sec.

fingolfin · 2017-01-08T19:12:04Z

Nice catch about that cache. In fact, there are only 331 entries, so setting this to 1000 just caches everything.

I just checked: a fully loaded PERFGRP is about 4.6 MB (though this is without my MemoryUsage fixes...). I think we should just remove the caching logic. And perhaps in the future revise it, once we got around to adding a proper generic "cache" system (possibly using weak pointers and stuff....) which we need for the HPC-GAP integration anyway.

The whole data is only about 4.5 MB in RAM. The implemented caching logic was not very clever either (e.g. not using a "last recently used" approach, but rather always throwing away the first 25 loaded PERFGRP entries).

ChrisJefferson · 2017-01-08T21:05:13Z

Coverage looks much better now -- no idea why one tiny bit of perf1.grp isn't covered... could you have a look and see if you think it's a profiling problem, or a GAP problem?

fingolfin · 2017-01-09T10:41:53Z

The unused code in perf1.grp really is not called, and there is a comment next to it which says # 1920.8 (otherpres.). I will investigate (just got the book by Plesken & Holt on my desktop)

fingolfin · 2017-01-09T13:38:00Z

Actually, there are a bunch of these entries with an "otherpres." comment next to them. The indices of these entries are always higher than the number of perfect groups of that order, so presumably we are looking at alternative presentations for some groups (but which?). So the real weird part is that some of them are used after all... indirectly. E.g. 1080.2 is used by PerfectGroup(69120,1).

I am not sure why 1920.8, 1344.3 and 1344.4 are there but not used. I could not yet identify their source in Holt/Plesken (but I only skimmed and didn't try hard, so it might be in there). Anyway, it seems harmless.

fingolfin · 2017-01-09T18:27:33Z

I think this is good now. Overall coverage is up by 3.55% and the grp dir has 90% coverage :-). Of course this makes it look at bit better than it is: There certainly could be more comprhensive checks. E.g. one could check whether the pre-computed group sizes and other properties are correct (by doing something like H:=Group(GeneratorsOfGroup(G));; Size(G)=Size(H);), check other group properties, etc. But it's a start.

ChrisJefferson

This looks good, with two small comments.

ChrisJefferson · 2017-01-11T22:53:51Z

grp/classic.gi

@@ -403,7 +403,7 @@ BindGlobal( "Oplus45", function()

    # construct the group without calling 'Group'
    g := [ phi*tau2, tau*eichler*delta ];
-    g:=List(g,i->ImmutableMatrix(f,i),true);
+    g:=List(g,i->ImmutableMatrix(f,i));


Did this code ever work, out of interest? What's that true supposed to do?

Yes, it worked until 814a0f6 broke it by making List validate its arguments more agressively. My guess is that the true in there was an accident.

ChrisJefferson · 2017-01-11T22:54:28Z

grp/glzmodmz.gi

+  g := Group(slmats);
+  mat := Concatenation(id{[nh+1..n]},-id{[1..nh]});
+  SetInvariantBilinearForm(g,rec(matrix:=mat));
+  return g;
 end);


I'm going to trust this is right.. I assume these is a test for it?

Yes -- indeed, writing the tests for Sp(2*n, Integers mod p^k) lead me to discover that the invariant form was missing for p=2, and hence I added this to make the tests pass. so this was TDD (test driven development) :-).

ChrisJefferson

Now approved

fingolfin · 2017-01-13T10:44:42Z

So, merge this? Rebase it? Should I do it myself?

fingolfin added 2 commits January 7, 2017 17:22

grp: remove trailing newlines from errors

b5c00b8

tests: add tests for various group constructors

2227604

fingolfin added 4 commits January 8, 2017 16:16

tests: construct all perfect groups

1b56939

grp: fix GO(+1,4,5)

cacf734

tests: verify forms of orthogonal and unitary groups

2b1ea6f

tests: move some fast tests from teststandard to testinstall

54ba846

Also updated some comments explaining why certain tests are excluded from testinstall.

fingolfin added 2 commits January 8, 2017 19:08

tests: cover two more ReeGroup/SuzukiGroup error messages

ba25a69

These error message (and also those for the classical groups) seem to vary quite a bit. We may want to unify them at some point.

grp: correct FreeAbelianGroup error message

f1c3f33

grp: remove extra space from error messages

b504e59

tests: add tests for basic group constructors

48acf38

grp: get rid of cache for perfect group data

901749c

The whole data is only about 4.5 MB in RAM. The implemented caching logic was not very clever either (e.g. not using a "last recently used" approach, but rather always throwing away the first 25 loaded PERFGRP entries).

fingolfin added 2 commits January 9, 2017 14:56

grp: set invariant form for Sp(2n,Integers mod 2^k)

30227dc

tests: construct more Sp & SO over ZmodnZ, check forms

3a85e11

fingolfin force-pushed the mh/coverage branch from 3178c0e to 3a85e11 Compare January 9, 2017 13:59

fingolfin changed the title ~~Some new test for group constructors~~ New tests for group constructors and some fixes Jan 9, 2017

ChrisJefferson reviewed Jan 11, 2017

View reviewed changes

ChrisJefferson approved these changes Jan 12, 2017

View reviewed changes

ChrisJefferson merged commit 9fffac5 into gap-system:master Jan 13, 2017

fingolfin deleted the mh/coverage branch January 25, 2017 13:44

fingolfin mentioned this pull request Sep 7, 2017

Release notes for GAP 4.9 #1699

Closed

olexandr-konovalov added this to the GAP 4.9.0 milestone Jan 29, 2018

olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New tests for group constructors and some fixes #1053

New tests for group constructors and some fixes #1053

fingolfin commented Jan 7, 2017 •

edited

Loading

codecov-io commented Jan 7, 2017 •

edited

Loading

ChrisJefferson commented Jan 8, 2017

fingolfin commented Jan 8, 2017

fingolfin commented Jan 8, 2017

fingolfin commented Jan 8, 2017

ChrisJefferson commented Jan 8, 2017

ChrisJefferson commented Jan 8, 2017

ChrisJefferson commented Jan 8, 2017

fingolfin commented Jan 8, 2017 •

edited

Loading

ChrisJefferson commented Jan 8, 2017

fingolfin commented Jan 9, 2017

fingolfin commented Jan 9, 2017

fingolfin commented Jan 9, 2017

ChrisJefferson left a comment

ChrisJefferson Jan 11, 2017

fingolfin Jan 11, 2017

ChrisJefferson Jan 11, 2017

fingolfin Jan 12, 2017

ChrisJefferson left a comment

fingolfin commented Jan 13, 2017

New tests for group constructors and some fixes #1053

New tests for group constructors and some fixes #1053

Conversation

fingolfin commented Jan 7, 2017 • edited Loading

codecov-io commented Jan 7, 2017 • edited Loading

Current coverage is 53.97% (diff: 100%)

ChrisJefferson commented Jan 8, 2017

fingolfin commented Jan 8, 2017

fingolfin commented Jan 8, 2017

fingolfin commented Jan 8, 2017

ChrisJefferson commented Jan 8, 2017

ChrisJefferson commented Jan 8, 2017

ChrisJefferson commented Jan 8, 2017

fingolfin commented Jan 8, 2017 • edited Loading

ChrisJefferson commented Jan 8, 2017

fingolfin commented Jan 9, 2017

fingolfin commented Jan 9, 2017

fingolfin commented Jan 9, 2017

ChrisJefferson left a comment

Choose a reason for hiding this comment

ChrisJefferson Jan 11, 2017

Choose a reason for hiding this comment

fingolfin Jan 11, 2017

Choose a reason for hiding this comment

ChrisJefferson Jan 11, 2017

Choose a reason for hiding this comment

fingolfin Jan 12, 2017

Choose a reason for hiding this comment

ChrisJefferson left a comment

Choose a reason for hiding this comment

fingolfin commented Jan 13, 2017

fingolfin commented Jan 7, 2017 •

edited

Loading

codecov-io commented Jan 7, 2017 •

edited

Loading

fingolfin commented Jan 8, 2017 •

edited

Loading