Fix finiteness condition in IsomorphismGroups

[zickgraf]

Currently, the arguments of IsomorphismGroups are not handled symmetrically:

gap> F := FreeGroup(1);
<free group on the generators [ f1 ]>
gap> IsFinite( F );
false
gap> S := SymmetricGroup( 3 );
Sym( [ 1 .. 3 ] )
gap> IsFinite( S );
true
gap> IsomorphismGroups( S, F );
fail
gap> IsomorphismGroups( F, S );
Error, cannot test isomorphism of infinite groups at /usr/lib/gap/lib/morpheus.gi:2151 called from
<function "IsomorphismGroups">( <arguments> )
 called from read-eval loop at *stdin*:5
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk>

After this commit, in both cases fail is returned.

[ChrisJefferson]

This does improve things, thanks!

Two small things:

1. While it is a simple case, it would be nice to add a .tst file.

2. I wonder if, just after this check, it would be worth adding a check if IsFinite(G) <> IsFinite(H) then return fail;. Obviously in the case you give in your example this is being handled, but I wonder if an explicit test would be worthwhile (seeing as we already know if the groups are finite anyway).

[fingolfin]

Actually, the condition is still not correct.

[fingolfin]

Yes, please add a test! And I am surprised that this change made your two tests pass -- I don't see how, given that it only triggered if both inputs were infinite?! Maybe I am misunderstanding something?

[zickgraf]

@fingolfin My change worked because later in the code there is the following check:

  if Size(G)<>Size(H) then
    return fail;

What I do not like about @fingolfin's suggestion is that one cannot easily read off in which cases the error is triggered exactly. That's why I'm in favor of @ChrisJefferson's second suggestion:

if not IsFinite(G) and not IsFinite(H) then
  Error("cannot test isomorphism of infinite groups");
fi;
if IsFinite(G) <> IsFinite(H) then
  return fail;
fi;

Do you agree or shall I implement @fingolfin's suggestion?

[wilfwilson]

Both pieces of code give the same answer in all cases...

if IsFinite(G) <> IsFinite(H) then
  return fail;
fi;
if not IsFinite(G) then
  Error(...)
fi;

if not IsFinite(G) and not IsFinite(H) then
  Error("cannot test isomorphism of infinite groups");
fi;
if IsFinite(G) <> IsFinite(H) then
  return fail;
fi;

...so I don't think it matters which one you choose.

(If G and H are both finite, they both do nothing.
If G and H have different finiteness, then the first bit of code immediately returns fail; the second bit of code skips the first condition, and returns fail on the second.
If G and H are both infinite, the first bit of code skips the first condition, and gives an error on the second; the second bit of code immediately gives the error.
In other words, the two if statements "commute".)

[zickgraf]

My current commit also gives the same answer in all cases because of the existing check Size(G)<>Size(H), so of course this is only a matter of code style.

[ChrisJefferson]

The only reason to bail earlier is there could be cases where we know group G is infinite, group H is finite (for example, H could be a permutation group), but we don't actually know H's size, and calcaulating it is expensive.

However, this is just an optimisation, and fixing the bug is obviously more important.

[codecov]

Codecov Report

Merging #3331 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3331      +/-   ##
==========================================
+ Coverage   85.25%   85.26%   +<.01%     
==========================================
  Files         697      697              
  Lines      344151   344150       -1     
==========================================
  Hits       293423   293423              
+ Misses      50728    50727       -1

Impacted Files	Coverage Δ
lib/morpheus.gi	84% <100%> (ø)	⬆️
src/hpc/threadapi.c	46.8% <0%> (+0.03%)	⬆️

[zickgraf]

I have pushed a test file and my suggestion from above. I hope that I did put the test file in the correct directory.

[coveralls]

Coverage increased (+0.0001%) to 85.075% when pulling d2f34a1 on zickgraf:fix_finiteness_condition into c07bf5e on gap-system:master.

[fingolfin]

I disagree with this solution, because before the Size check, there is an IsAbelian check, and that can easily run into an infinite loop for infinite groups.

[fingolfin]

Where "this solution" referred to "leave it to the Size check later on". But it seems you now did what I suggested anyway, so that should be fine.

[fingolfin]

Could you please squash this into a single commit?

[zickgraf]

Done!