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.

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

Improve transformations code and tests #727

Merged
merged 65 commits into from Apr 13, 2016

Conversation

james-d-mitchell
Copy link
Contributor

This pull requests contains many minor improvements in the transformations code and its tests. It does not contain any changes in functionality except where this involved fixing a bug.

This came out of wanting to remove the randomness in the test file trans/testinstall/trans.tst, and to get trans/testinstall/trans.tst to run in a more reasonable amount of time (as mentioned in Issue #573). In the process of doing this I rewrote some of the C functions for transformations to improve their complexity, reduce the length of the code, and to make them more robust (by checking their arguments more thoroughly). I also removed several C/GAP functions that weren't used or documented, cleaned up the whitespace is src/trans.c, lib/trans.gi, and lib/trans.gd, reorganised src/trans.c and added more comments, and fixed some bugs and inconsistencies.

The file trans/testinstall/trans.tst now runs in about 600ms (on my laptop), and tests almost all of the code in src/trans.c and lib/trans.gi, and doesn't contain any randomness, except to test the function RandomTransformation.

The whitespace changes are separate from the other changes. I realise that there are a large number of commits in this PR, I'm happy to rebase this to whatever anyone wants.

Alexander Konovalov and others added 30 commits April 11, 2016 17:27
Better checking of the arguments, and reduce the degree in case the
defining lists contain trailing fixed points.
Make functions more robust by checking their args, remove randomness
from test files, and ensure 100% code coverage.
Remove trailing whitespace and use uniform code formatting.
Make functions more robust by checking their args, remove randomness
from test files, and ensure 100% code coverage.
The new method has lower complexity than the old method, which was
rubbish.
To avoid duplicate code.
Remove trailing whitespace and use uniform code formatting.
Make functions more robust by checking their args, remove randomness
from test files, and ensure 100% code coverage.
Remove trailing whitespace and use uniform code formatting.
The new method is a bit faster and uses less memory.
And tests for COMPONENTS_TRANS and NR_COMPONENTS_TRANS. Also update
NR_COMPONENTS_TRANS to give an error if the arg is not a transformation.
The output of the previous code was not well-defined, or used, so I
removed it.
So that functions with related functionality are next to each other, in
clearly marked sections.

Rename:
  * IMAGE_TRANS -> IMAGE_LIST_TRANS_INT
  * PERM_LEFT_QUO_TRANS_NC -> PermLeftQuoTransformationNC
  * INV_TRANS -> InverseOfTransformation

Remove the GAP versions of the last two, and update the tests.
Namely those of the identity, and a permutation.
Remove unnecessary GAP level versions of C functions, and rename the
C function for:

* RESTRICTED_TRANS -> RestrictedTransformationNC
* PERM_IMG_TRANS -> PermutationOfImage
* IS_INJECTIVE_LIST_TRANS -> IsInjectiveListTrans
* INDEX_PERIOD_TRANS -> IndexPeriodOfTransformation
and properly check their arguments.
Remove now redundant code, declarations, fix minor problems, and add
checks.
and rename RestrictedTransformationNC -> RestrictedTransformation, since
the C function anyway checks its args fully.
@markuspf
Copy link
Member

Running

gap> Test("tst/teststandard/trans.tst");

runs into many instances of IS_INJECTIVE_LIST_TRANS must have a value errors on my machine.

@markuspf
Copy link
Member

Also, at the risk of sounding too optimistic: How far are we away from consolidating permutat.c and trans.c?

@james-d-mitchell
Copy link
Contributor Author

@markuspf the old trans.tst in teststandard should be deleted. I just parked it there to make sure it still ran while I was working on these changes.

There are actually not as many functions in permutat.c and trans.c with overlapping functionality as you might think. So, I don't think this would be all that difficult to do. I'd suggest that that goes into another pull request though.

@ChrisJefferson
Copy link
Contributor

I had a look through this pull request, and generally speaking it all looks good (obviously there are many things). There could probably be more tests (although the tests are as good as they've ever been before).

@james-d-mitchell
Copy link
Contributor Author

There are 3020 lines of new tests, testing 95.4 % of the lines in trans.c and 98% of the lines in trans.gi. The only untested code is some that can't be entered in trans.c (but that should be there anyway) and some lines:

end);

in trans.gi.

I'm going to interpret "as good as they've ever been" as "better than before" :)

Joking aside: I think I know what you mean, the tests for any given function could probably include more complicated/different transformations.

If you have any specific suggestions for further tests, then I'd be happy to add them.

@markuspf
Copy link
Member

I think we'll have to remove the trans.tst from teststandard, otherwise tests will fail on jenkins, other than that I could just hit the merge button.

This file contains the old tests for transformations.
@james-d-mitchell
Copy link
Contributor Author

I've removed the old test file.

@markuspf markuspf merged commit 307bd2a into gap-system:master Apr 13, 2016
@olexandr-konovalov
Copy link
Member

@james-d-mitchell besides the problem with 32-bit builds mentioned by me earlier today, namely

########> Diff in /Volumes/hudson-fs/workspace/GAP-dev-compilers/GAPCOPTS/32bu\
ild/GAPREADLINE/readline/label/gcc48/GAP-git-snapshot/tst/testinstall/trans.ts\
t:119
# Input is:
RANK_TRANS_LIST(Transformation([65537], [1]), 
                Concatenation([1], [65536 .. 70000]));
# Expected output:
4464
# But found:
4465
########
trans.tst              38670           1918   ( next ~143 sec )

there is a problem with loading all packages, for example:

Error, Variable: 'PERM_LEFT_QUO_TRANS_NC' must have an assigned value in
  return PERM_LEFT_QUO_TRANS_NC
 ; at /home/hudson/hudson/workspace/GAP-dev/GAPCOPTS/64build/GAPGMP/gmp/GAPTAR\
GET/packagesload/label/graupius/GAP-git-snapshot/pkg/semigroups-2.7.4/gap/setu\
p.gi:725 called from 
LambdaPerm( s 
 ) at /home/hudson/hudson/workspace/GAP-dev/GAPCOPTS/64build/GAPGMP/gmp/GAPTAR\
GET/packagesload/label/graupius/GAP-git-snapshot/pkg/semigroups-2.7.4/gap/lamb\
da-rho.gi:214 called from
LambdaOrbSchutzGp( o, m 
 ); at /home/hudson/hudson/workspace/GAP-dev/GAPCOPTS/64build/GAPGMP/gmp/GAPTA\
RGET/packagesload/label/graupius/GAP-git-snapshot/pkg/semigroups-2.7.4/gap/lam\
bda-rho.gi:296 called from
LambdaOrbStabChain( LambdaOrb( S ), m 
 ) at /home/hudson/hudson/workspace/GAP-dev/GAPCOPTS/64build/GAPGMP/gmp/GAPTAR\
GET/packagesload/label/graupius/GAP-git-snapshot/pkg/semigroups-2.7.4/gap/semi\
groups-acting.gi:186 called from
list[i][deg + 1] 
in semi
  at /home/hudson/hudson/workspace/GAP-dev/GAPCOPTS/64build/GAPGMP/gmp/GAPTARG\
ET/packagesload/label/graupius/GAP-git-snapshot/pkg/automgrp/gap/listops.gi:56\
 called from
AG_IsCorrectAutomatonList( list, false 
 ) at /home/hudson/hudson/workspace/GAP-dev/GAPCOPTS/64build/GAPGMP/gmp/GAPTAR\
GET/packagesload/label/graupius/GAP-git-snapshot/pkg/automgrp/gap/automsg.gi:6\
0 called from
...  at /home/hudson/hudson/workspace/GAP-dev/GAPCOPTS/64build/GAPGMP/gmp/GAPT\
ARGET/packagesload/label/graupius/GAP-git-snapshot/pkg/automgrp/gap/groups.g:
45
you can 'return;' after assigning a value
brk> 

Should I wait for the new version of Semigroups which will resolve this?

@olexandr-konovalov
Copy link
Member

Another strange thing happening in the test of manual examples (make testmanuals) under OS X:

=========OUTPUT START: testmanuals with autoloaded packages=========

#W bad bag id 2907264 found, 0 saved
#W bad bag id 4032404 found, 0 saved
#W bad bag id 4033020 found, 0 saved
Checking tut, Chapter 1
Checking tut, Chapter 2
Checking tut, Chapter 3
Checking tut, Chapter 4
Checking tut, Chapter 5
Checking tut, Chapter 6
Checking tut, Chapter 7
Checking tut, Chapter 8
============================================================
NO DIFFERENCES IN TUTORIAL EXAMPLES (DEFAULT PACKAGES)
============================================================

#W bad bag id 2907264 found, 0 saved
#W bad bag id 4032404 found, 0 saved
#W bad bag id 4033020 found, 0 saved
Checking ref, Chapter 1
Checking ref, Chapter 2
Checking ref, Chapter 3

@james-d-mitchell
Copy link
Contributor Author

Thanks for the reports, I'll investigate.

I think I know what the problem is with the 32 bit build, but I can't get
gap to compile in 32-bit mode (the linker complains about gmp), so I'm not
sure how I can test if I am correct. Is there any machine running 32bit gap
that I can log into, so I can try to resolve this?

@james-d-mitchell
Copy link
Contributor Author

I've looked at the "bad bag" issue, and I don't think this is related to the changes I made in the transformations code. If I go back to changeset e948e84 (in master before PR #727 was merged), and do:

gap -A
[...]
gap> SaveWorkspace("tmp.ws");
true
gap> quit;

gap -A -L tmp.ws
[...]
gap> SaveWorkspace("tmp.ws");
#W bad bag id 4303145168 found, 0 saved
true

Maybe this is a different issue, not what triggers the "bad bag" messages when you do make make testmanuals. But I didn't change anything in the loading/saving of transformations in trans.c as part of PR #727 (except maybe some whitespace changes), so I think if this is related to the transformations code, then it existed before the recent changes in PR #727.

@james-d-mitchell
Copy link
Contributor Author

I have an updated version of Semigroups (2.7.5) which fixes the issue you mention above, ready to release. But some of the test fail against the master branch of GAP due to Issue #730, I will release a new version of Semigroups when #730 is resolved.

@james-d-mitchell
Copy link
Contributor Author

@alex-konovalov I have fixed the issue with RANK_TRANS_LIST in PR #732.

@olexandr-konovalov
Copy link
Member

@james-d-mitchell: thanks. With respect to "bad bags", @ChrisJefferson was going to look into it.

@james-d-mitchell
Copy link
Contributor Author

Great thanks!

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.

None yet

4 participants