New Functionality: Cannon/Holt automorphisms and others

[hulpke]

Enhancements for next release:
Cannon/Holt automorphism group routine (as it is usually much faster) if nontrivial radical.
Use automorphism group for isomorphism testing.
Improved IntermediateSubgroups routine.

[codecov-io]

Current coverage is 49.12% (diff: 2.74%)

Merging #878 into master will decrease coverage by 0.18%

@@             master       #878   diff @@
==========================================
  Files           422        423     +1   
  Lines        228403     229166   +763   
  Methods        3448       3448          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits         112638     112579    -59   
- Misses       115765     116587   +822   
  Partials          0          0          

Powered by Codecov. Last update 8f5e7f4...93c0cbd

[fingolfin]

Great! However, I wonder if there are any tests which actually exercise this code. I recommend adding some, else the result of the tes suite is meaningless, isn't it?

[hulpke]

@fingolfin Yes, there now are tests, but codecov grabbed it before.

[markuspf]

On Fri, Aug 05, 2016 at 07:59:41AM -0700, Alexander Hulpke wrote:

@fingolfin Yes, there now are tests, but codecov grabbed it before.

Hi,

codecov will update the results if there are commits added/pull requests
amended.

Cheers,
Markus

[fingolfin]

@hulpke Did you push the MTC changes here on purpose? (My guess is that you are re-using your "additions" branch for unrelated work... but a pull request always tracks the state of the branch it is based on, so things end up being mixed together).

[hulpke]

@fingolfin
It is on purpose -- the additions branch is material that I intend to go into the next major release. (I know -- in the best of all worlds these would be different feature branches, but I'm not yet at that level of enlightenment.)

[fingolfin]

Ok, that's fine by me :-)

[fingolfin]

The new attribute CharacteristicSubgroups conflicts with CRISP, which also declares this, and provides several methods.

[hulpke]

@fingolfin
Ah -- what is a clean way to resolve this? Both package and library declare CharcteristicSubgroups in the same (obvious....) way.

[olexandr-konovalov]

Regarding PatheticIsomorphism (nice name, btw), I am curious whether in the case when groups are non-isomorphic it will return fail faster than IsomorphismGroups ?

[hulpke]

@alex-konovalov
Well, IsomorphismGroups now will call PatheticIsomorphism in cases that there need to be at least 4 generators, as the other function will perform very badly.
If you suspect the groups to be non-isomorphic, it might not be the best function as it always forces through the whole automorphism group, which could be expensive -- the implicit assumption is that all plausible invariant tests have been done and we're willing to bet that the group are isomorphic.

[fingolfin]

Starting GAP from this branch gives a syntax warning:

Syntax warning: Unbound global variable in PATH/gap/lib/grpnames.gi:466

The fix is obvious enough :)

As to how to resolve the conflict with CharacteristicSubgroups: I guess we'd need to coordinate this carefully with the author of the CRISP package, @bh11 . One possible path forward could be this:

• We add CharacteristicSubgroups to the GAP library.
• At the same time, or soon after, a new CRISP release is made, which surrounds its DeclareAttribute call by an if not IsBound(CharacteristicSubgroups).
• Once that version has been picked up by the package distribution, we can make a new release of GAP, with that version of crisp bundled
• sometime later, a new crisp version could remove its DeclareAttribute(CharacteristicSubgroups, ...) completely, and list in its PackageInfo.g the relevant GAP version as minimal required.

It's not pretty, but the best I can think of right now... Oh, and of course it does not take care of interactions between the various methods for CharacteristicSubgroups... i.e. if computing automorphism groups uses CharacteristicSubgroups, then it could change in behavior depending on whether CRISP is loaded or not. As long as it only gets faster / better, that's of course fine; but we'd have to carefully test for that, I guess...?!

[olexandr-konovalov]

At the same time, or soon after, a new CRISP release is made, which surrounds its DeclareAttribute call by an if not IsBound(CharacteristicSubgroups).

Just my 1p: If this results in just a warning, "soon after" is OK, but if this breaks package loading (e.g. make testinstall fails when it attempts to load all packages) then it should be "at the same time" or "immediately after".

[fingolfin]

@markuspf Can you tell us which tests the results on codecov.io are based on? Without this, data as on e.g. https://codecov.io/gh/gap-system/gap/pull/878/compare is mostly useless... :-)

[hulpke]

@fingolfin At the moment CharacteristicSubgroups is there for potential use in isomorphisms, but the automorphism group calculation does not use it. I suggest this to be a plausible dependency, a routine to compute automorphisms inthe first place could only get subgroups (say verbal subgroups) that have to be characteristic, but would not be allowed to call CharacteristicSubgroups in the first place.

[bh11]

@fingolfin I guess this should be handled in DeclareAttribute, probably in a way similar to DeclareOperation (which currently issues a warning via INFO_DEBUG and ignores the second declaration).

Maybe it even makes sense to raise the info level to 2 (so that identical DeclareAttribute/DeclareOperation calls don't normally show up – this might help reducing package dependencies and backward compatibility issues).

Of course, it's no problem to make a new version of CRISP once ChearacteristicSubgroups is declared in the main library.

[olexandr-konovalov]

@fingolfin just now, if you open https://codecov.io/gh/gap-system/gap/pull/878/compare, it says

Compare 80a0966 ... +8 ... ebe9955

Those commit hashes are clickable and they will lead you to corresponding commits. You can explore them and see that at the moment it compares 80a0966 which is the revision on which @hulpke's branch is based, with ebe9955 which is currently the mist recent commit he made in his branch.

[fingolfin]

@alex-konovalov Yes, of course, that's the obvious bit, I didn't need an explanation of that :-). What is not obious (to me, at least), is "which tests the results on codecov.io are based on" -- i.e. is this testinstall? teststandard? something else? In other words: What does the absolute coverage report refer to?

[markuspf]

On Tue, Aug 16, 2016 at 10:42:44AM -0700, Max Horn wrote:

@alex-konovalov Yes, of course, that's the obvious bit, I didn't need an
explanation of that :-). What is not obious (to me, at least), is "which tests
the results on codecov.io are based on" -- i.e. is this testinstall?
teststandard? something else? In other words: What does the absolute coverage
report refer to?

All the tests that are run in Travis, i.e. whats shown in "builds":
https://codecov.io/gh/gap-system/gap/commit/ebe99553227a8648cea81e482771e03161754010/builds

(albeit the place to find it again not obvious).

[hulpke]

I second @bh11 in suggesting that DeclareAttribute should be forgiving in such a case. This king of declaration moving is going to happen again.

[hulpke]

I have moved the MTC addition in the newmtc branch, pull #892

[olexandr-konovalov]

@hulpke I have an impression that this periodically triggers the following

########> Diff in /home/hudson/hudson/workspace/GAP-pkg-update-master-quicktes\
t/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/standard/label/graupius/GAP-pkg-update\
-master-snapshot/tst/teststandard/grpperm.tst:223
# Input is:
IsomorphismGroups(gp1,gp2)<>fail;
# Expected output:
true
# But found:
Error, reached the pre-set memory limit
(change it with the -o command line option)
########

when all packages are loaded. I will try to narrow this down, but in the meantime could you please try if you also can reproduce this?