Address @hungaborhorvath's pull requests

[markuspf]

@hungaborhorvath has submitted a number of pull requests, most of which have been commented on by multiple people. Since the issues are somehow connected, we should decide on what to do with the pull requests. The numbers are listed below for convenience.

• Some small enhancements on Sylow and Hall subgroup computations #535
• Enhance MaximalNormalSubgroups for finite solvable groups #552
• Add NormalHallSubgroups #561
• Add ComplementClassesRepresentatives for all groups #563
• Add FrattiniSubgroup for nilpotent groups #583
• Add MinimalNormalSubgroups and Socle for all nilpotent groups #606

[hulpke]

#535, #561 are fine

#552, #583, #606 circumvent the method selection to force a test for nilpotence first. I'm uncomfortable with such installations, unless a clear benefit and lack of cost in inapplicable cases can be demonstrated,
The actual method code is fine.
My inclination would be to accept these pull requests, followed by a change of these installations (i.e. the methods installed for IsNilpotentGroup).

#563 is a hopeless method (which the included tests actually don't test) and which I'm inclined to reject: If both N and G?n are not solvable, it is unlikely that a subgroup lattice computation ever will terminate.

[markuspf]

Thank you for the feedback, @hulpke.

I merged #535 and #561 and we will see whether the more extensive tests run on jenkins turn up any problems. I'll have a closer look at the others soon (unless someone else beats me to it or objects loudly)

[stevelinton]

I think #549 is not the PR you mean.

[markuspf]

@stevelinton indeed. I just removed it.

[stevelinton]

Regarding #552, #583 and #606, I share @hulpke's concern about circumventing method selection in this way.

I think the test is in the wrong place. If you have two methods for solving a problem, where the first requires some Properties of the arguments and the second is slower (or even is just a No Method Found) it may well be sensible to try and explicitly check some of the Properties before calling the slower method. Specifically when that check will never fail when the slower method succeeds, and when the check is likely to be quicker than the slower method.

However I think the place to put that check is in the slower method (or as a fallback method if there is no slower method). In particular if the slower method is changed in any way, the conditions for doing the check need to be re-evaluated.

[markuspf]

I will try to merge #552, #583 and #606 with the proposed change that the methods are only installed for IsNilpotentGroup not for IsGroup.

[markuspf]

Mhm actually the MaximalNormalSubgroups code in #552 could be broken up futher into IsAbelian and IsSolvableGroup?

[markuspf]

I just merged #552, but split up the installed method for maximal subgroups into a case for abelian groups and a case for solvable groups.

[hungaborhorvath]

Thank you very much for checking these PRs.

@markuspf If you break up the code to be installed for only groups having the property, you do not need the artificial rank increase, anymore. Namely, RankFilter( IsGroup and IsAbelian ) - RankFilter( IsGroup ) should just be replaced by 0. Or should I put in a pull request about this after all these are merged?

[markuspf]

@hungaborhorvath I will just remove the rank increase manually. Thanks for pointing that out.

[olexandr-konovalov]

We have two new diffs now:

########> Diff in /home/hudson/workspace/GAP-dev-compilers/GAPCOPTS/64build/GA\
PREADLINE/noreadline/label/gcc49/GAP-git-snapshot/tst/testinstall/opers/Maxima\
lNormalSubgroups.tst:32
# Input is:
List(MaximalNormalSubgroups(D2), StructureDescription);
# Expected output:
[ "D180", "D180", "C180" ]
# But found:
[ "C180", "D180", "D180" ]
########
########> Diff in /home/hudson/workspace/GAP-dev-compilers/GAPCOPTS/64build/GA\
PREADLINE/noreadline/label/gcc49/GAP-git-snapshot/tst/testinstall/opers/Maxima\
lNormalSubgroups.tst:40
# Input is:
Length(MaximalNormalSubgroups(G));
# Expected output:
7
# But found:
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 3rd choice method found for `ConjugacyClasses' on 1 arguments
########

While the 1st one is just the ordering, the 2nd one is strange. How did we miss it with Travis CI?

[hungaborhorvath]

@alex-konovalov @markuspf A RedispatchOnCondition needs to be added for all of these new methods.

In the tests that I have added I assumed that MaximalNormalSubgroups will check if the group (even if infinite) is Abelian or Solvable. This did not happen now, and GAP did not find any methods.

This will be an issue in all other PRs, as well.

[markuspf]

I am not sure whether RedispatchOnCondition (which I hadn't heard of until an hour ago) would not also constitute trying to trick method selection?

My impression was that the methods were only supposed to be selected if the appropriate properties of the group were already known.

Maybe @hulpke and/or @stevelinton can comment on that.

[stevelinton]

Redispatch on Condition is more-or-less the official way to deal with these situations, along the lines I suggested above.

Critically it is basically guaranteed to be called last, so it can’t get in the way of any method that might actually work.

Steve

On 28 Mar 2016, at 20:25, Markus Pfeiffer notifications@github.com wrote:

I am not sure whether RedispatchOnCondition (which I hadn't heard of until an hour ago) would not also constitute trying to trick method selection?

My impression was that the methods were only supposed to be selected if the appropriate properties of the group were already known.

Maybe @hulpke and/or @stevelinton can comment on that.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub