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

StructureDescriptions fix #985

Merged

Conversation

hungaborhorvath
Copy link
Contributor

@hungaborhorvath hungaborhorvath commented Dec 5, 2016

This splits the current general StructureDescription code for an abelian for a finite and for a finite simple method, with redispatches.

Further, it adds nice monomorphism in the proper way (into grpnice.gi), while removes it from grpnames.gi.

Corrects problematic tests, and fixes the StructureDescription part of #973.

Added some more tests to increase the code coverage of grpnames.gi . Now all lines (except for the ones for which I have no idea if an example even exists for) are covered for StructureDescription, DirectFactors and SemidirectDecompositions.

Manual entries are cleaned up in these three functions and in some of the functions they use in grpnames.gd (for easier future documentation).

@hungaborhorvath
Copy link
Contributor Author

hungaborhorvath commented Dec 6, 2016

Hm, insertsep and cycsaspowers were internal functions for StructureDescription. Now, I just moved them out with BindGlobal to avoid code duplication, because both the abelian and the finite method use these functions. But maybe their names should be changed? Is there a convention to naming such functions (like e.g. all small letters or all capital letters)?

@codecov-io
Copy link

codecov-io commented Dec 6, 2016

Current coverage is 49.63% (diff: 98.88%)

Merging #985 into master will increase coverage by 0.12%

@@             master       #985   diff @@
==========================================
  Files           424        424          
  Lines        223266     223277    +11   
  Methods        3430       3430          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits         110545     110821   +276   
+ Misses       112721     112456   -265   
  Partials          0          0          

Powered by Codecov. Last update 05829fe...11d1587

@hulpke
Copy link
Contributor

hulpke commented Dec 6, 2016

@hungaborhorvath
There is no general policy on names.
I have tended to name such utility functions as uppercase, a name that clearly sounds technical and some prefix that indicates where they belong to. E.g. in your case SDinsertSep

@hungaborhorvath
Copy link
Contributor Author

@hulpke Thank you, I have renamed these internal functions to make it more clear that they are really just internal functions. I squashed the commit into a previous one to make history more clear.

@alex-konovalov this should fix #973 and also the discrepancy in bugfix.tst with no packages.

BTW, currently all lines in grpnames.gi for DirectFactors, SemidirectDecompositions and StructureDescription are covered, except those that I have no idea if there even exists an example for. In the future I plan to take a look at the other functions occurring in grpnames.gi and will write tests to cover those, as well.

@hungaborhorvath hungaborhorvath force-pushed the SemidirectDecompositionsFix branch 4 times, most recently from f290c7a to 194903e Compare December 11, 2016 09:51
@olexandr-konovalov
Copy link
Member

@hungaborhorvath thanks - we still need to review this, but now the discrepancy in bugfix.tst with no packages is the only thing that breaks make teststandard.

@olexandr-konovalov
Copy link
Member

For the record, there are now 12 diffs in manual examples with default set of packages. I think they appeared after recent merges by @hungaborhorvath and @hulpke. Since there will be further changes, I may wait with fixing them till the code will stabilise a bit more, also trying to make some of the examples involved more reproducible:

########> Diff in [ "./group.xml", 831, 838 ]
# Input is:
blocks[1];
# Expected output:
[ (1,2)(3,4)(5,6)(7,8), (1,3)(2,4)(5,7)(6,8), (1,4)(2,3)(5,8)(6,7), 
 (1,5)(2,6)(3,7)(4,8), (1,6)(2,5)(3,8)(4,7), (1,7)(2,8)(3,5)(4,6), 
 (1,8)(2,7)(3,6)(4,5) ]
# But found:
[ (1,2)(3,4)(5,6)(7,8), (1,3)(2,4)(5,8)(6,7), (1,4)(2,3)(5,7)(6,8), 
 (1,5)(2,6)(3,8)(4,7), (1,6)(2,5)(3,7)(4,8), (1,7)(2,8)(3,6)(4,5), 
 (1,8)(2,7)(3,5)(4,6) ]
########
########> Diff in [ "./group.xml", 1171, 1179 ]
# Input is:
IsomorphismGroups( niceaut, SymmetricGroup( 4 ) );
# Expected output:
[ (1,4,2,3), (1,5,4)(2,6,3), (1,2)(3,4), (3,4)(5,6) ] -> 
[ (1,2,4,3), (2,4,3), (1,4)(2,3), (1,2)(3,4) ]
# But found:
[ (1,4,2,3), (1,5,4)(2,6,3), (1,2)(3,4), (3,4)(5,6) ] -> 
[ (1,4,3,2), (1,4,2), (1,3)(2,4), (1,4)(2,3) ]
########
########> Diff in [ "./../../lib/grp.gd", 1131, 1135 ]
# Input is:
ConjugacyClassesMaximalSubgroups(g);
# Expected output:
[ AlternatingGroup( [ 1 .. 4 ] )^G, Group( [ (1,2,3), (1,2) ] )^G, 
 Group( [ (1,2), (3,4), (1,3)(2,4) ] )^G ]
# But found:
[ Group( [ (2,4,3), (1,4)(2,3), (1,3)(2,4) ] )^G, 
 Group( [ (3,4), (1,4)(2,3), (1,3)(2,4) ] )^G, 
 Group( [ (3,4), (2,4,3) ] )^G ]
########
########> Diff in [ "./../../lib/grp.gd", 1178, 1182 ]
# Input is:
MaximalSubgroupClassReps(g);
# Expected output:
[ Alt( [ 1 .. 4 ] ), Group([ (1,2,3), (1,2) ]), 
 Group([ (1,2), (3,4), (1,3)(2,4) ]) ]
# But found:
[ Group([ (2,4,3), (1,4)(2,3), (1,3)(2,4) ]), Group([ (3,4), (1,4)
 (2,3), (1,3)(2,4) ]), Group([ (3,4), (2,4,3) ]) ]
########
########> Diff in [ "./grpfp.xml", 266, 279 ]
# Input is:
h := IsomorphismPermGroup( g );
# Expected output:
[ a, b ] -> [ (1,2)(4,5), (2,3,4) ]
# But found:
[ a, b ] -> [ (1,2)(3,5), (2,3,4) ]
########
########> Diff in [ "./../../lib/grp.gd", 4156, 4167 ]
# Input is:
iso := IsomorphismFpGroupByGenerators( g, [ (1,2), (1,2,3,4,5) ] );
# Expected output:
#I  the image group has 2 gens and 4 rels of total length 50
[ (1,2), (1,2,3,4,5) ] -> [ F1, F2 ]
# But found:
#I  the image group has 2 gens and 5 rels of total length 39
[ (1,2), (1,2,3,4,5) ] -> [ F1, F2 ]
########
########> Diff in [ "./../../lib/grp.gd", 4156, 4167 ]
# Input is:
RelatorsOfFpGroup( fp );
# Expected output:
[ F1^2, (F2*F1*F2^-2*F1)^3, 
 F2*F1*F2^-1*(F1*F2)^2*F2^2*(F1*F2^-1)^2*F2^-1*F1, 
 (F2*F1*F2^-1*F1)^2*F2^-1*F1*F2^2*F1*F2^-2*F1*F2*F1 ]
# But found:
[ F1^2, F2^5, (F2^-1*F1)^4, (F1*F2^-1*F1*F2)^3, (F1*F2^2*F1*F2^-2)^2 ]
########
########> Diff in [ "./../../lib/grp.gd", 4187, 4196 ]
# Input is:
iso := IsomorphismFpGroupByGenerators( M12, gens );;
# Expected output:
#I  the image group has 3 gens and 21 rels of total length 459
# But found:
#I  the image group has 3 gens and 20 rels of total length 464
########
########> Diff in [ "./../../lib/grp.gd", 4187, 4196 ]
# Input is:
iso := IsomorphismFpGroupByGenerators( M12, gens );;
# Expected output:
#I  the image group has 3 gens and 18 rels of total length 337
# But found:
#I  the image group has 3 gens and 19 rels of total length 491
########
########> Diff in [ "./../../lib/grp.gd", 4238, 4245 ]
# Input is:
iso := IsomorphismFpGroupByGenerators( M12, gens : 
                                         method := "fast" );;
# Expected output:
#I  the image group has 3 gens and 153 rels of total length 3374
# But found:
#I  the image group has 3 gens and 179 rels of total length 4099
########
########> Diff in [ "./../../lib/grp.gd", 4257, 4279 ]
# Input is:
iso := IsomorphismFpGroupByGenerators( G, gens );;
# Expected output:
#I  the image group has 2 gens and 10 rels of total length 122
# But found:
#I  the image group has 2 gens and 9 rels of total length 94
########
########> Diff in [ "./../../lib/grp.gd", 4257, 4279 ]
# Input is:
ConstituentsCompositionMapping(iso);
# Expected output:
[ <action isomorphism>, 
 [ (2,3,4)(5,6)(8,9,10), (1,2,3,5)(6,7,8,9) ] -> [ F1, F2 ] ]
# But found:
[ <action isomorphism>, 
 [ (2,3,5,9,16,29)(4,7,13,24,19,32)(6,11,20,34,40,57)(8,15,28,46,42,
       59)(10,18,25,41,49,67)(12,22,37,53,48,66)(14,26,31)(17,30,35,
       50,58,38)(21,36,33)(23,39,56)(27,44,61,72,43,60)(45,62,51,68,
       54,70)(47,64,73)(52,69)(55,71,75,78,77,76)(65,74), 
     (1,2,4,8)(3,6,12,23)(5,10,19,33)(7,14,27,45)(9,17,18,31)(11,21,
       16,28)(13,25,42,57)(20,35,51,67)(22,38,55,70)(24,40,26,43)(29,
       37,54,39)(30,47,65,68)(32,48)(34,49,36,52)(41,58,56,61)(44,50,
       53,64)(46,63,69,59)(60,66,75,79)(62,73,72,77)(71,76,80,74) 
    ] -> [ F1, F2 ] ]
########

@hungaborhorvath
Copy link
Contributor Author

hungaborhorvath commented Dec 15, 2016

@alex-konovalov Thanks. I have rebased this PR to latest master, and ran a check on bugfix.tst with and without packages (BTW, with packages it takes 1.6 times more to run these tests).

I confirm again that this PR solves the old issue with bugfix.tst, but has a new one in current master (see #1022), which is not related to my changes....

I am also running make teststandard, but that will take longer.

The manual diffs above are not related to my changes. I believe they are due to the changes made by @hulpke . They all look harmless.

Edit: #1022 is a non-issue, I have not compiled my packages properly. There are no issues with bugfix.tst after this patch.

@hungaborhorvath
Copy link
Contributor Author

@alex-konovalov How should one reference attributes in the manual? E.g. is <Ref Func="ConjugacyClassesSubgroups"/> or <Ref Func="StructureDescription"/> correct, or should they start by <Ref Attr= ?

@olexandr-konovalov
Copy link
Member

@hungaborhorvath yes, while there will be no error reported while building the manual, it's consistent to use Attr for attributes.

The full list of what else can be used there could be seen in the Ref definition at http://www.gap-system.org/Manuals/pkg/GAPDoc-1.5.1/doc/chap3.html#X865F20E386B6DA49

<!ELEMENT Ref EMPTY>
<!ATTLIST Ref Func      CDATA #IMPLIED
              Oper      CDATA #IMPLIED
              Meth      CDATA #IMPLIED
              Filt      CDATA #IMPLIED
              Prop      CDATA #IMPLIED
              Attr      CDATA #IMPLIED
              Var       CDATA #IMPLIED
              Fam       CDATA #IMPLIED
              InfoClass CDATA #IMPLIED
              Chap      CDATA #IMPLIED
              Sect      CDATA #IMPLIED
              Subsect   CDATA #IMPLIED
              Appendix  CDATA #IMPLIED
              Text      CDATA #IMPLIED

              Label     CDATA #IMPLIED
              BookName  CDATA #IMPLIED
              Style (Text | Number) #IMPLIED>  <!-- normally automatic -->

@hungaborhorvath
Copy link
Contributor Author

@alex-konovalov Thank you. I think in the last commit I managed to correct several inconsistencies in the documentation of StructureDescription, and hopefully cleaned up most of the other entries in grpnames.gd that can become part of the manual at a possibly later point.

In particular for StructureDescription.
Prepare manual entries for DirectFactorsfFGroup and SemidirectDecompositions
for possibly documenting them later.
## Adds the <A>obj</A> to the list <A>list</A>. If CanEasilySortElements
## is true for <A>list</A> and <A>list</A> is a set, then AddSet is used
## instead of Add. Does not return anything, but changes <list>.
## </Description>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CanEasilySortElements, AddSet and Add should use a proper markup (<Ref ... elements when they could be cross-referenced, or <C>...</C> otherwise.

## presentation. Note, that this may take a long time.
## presentation. Note, that this may take a very long time if <A>G</A> has
## many normal subgroups, e.g. if <A>G</A>/<A>G</A>' has many cyclic
## factors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be <M><A>G</A>/<A>G</A>'</M> ?

@@ -49,6 +51,8 @@ gap> List(DirectFactorsOfGroup(G), IdGroup);
[ [ 8, 3 ], [ 8, 3 ] ]
gap> List(DirectFactorsOfGroup(SmallGroup(64,226):useKN), IdGroup);
[ [ 8, 3 ], [ 8, 3 ] ]
gap> List(DirectFactorsOfGroupKN(SmallGroup(8,5)), IdGroup);
[ [ 2, 1 ], [ 2, 1 ], [ 2, 1 ] ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I was confused when I've seen DirectFactorsOfGroupKN and assumed that I've missed that we have introduced some other abbreviation like "NC". Then I figured out that this stands for "Kayal-Nezhmetdinov". Maybe I'd prefer DirectFactorsOfGroupByKN more.


## this currently takes a veeeeeery long time (about 20 minutes)
#gap> StructureDescription(Ree(27));
#"Ree(27)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This had been discussed in #987. Is this example critical to achieve code coverage, or could be easily removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it work with a hack, see above tst/testinstall/opers/StructureDescription.tst:87. This would be cleaner, but I agree with what @hulpke says in #987, that it is not a real usecase. I could just remove this if you like (but it is commented out anyway).

Just to note that this example was not entirely in vain: the particular line this runs in StructureDescription was incorrect, and this test made me correct it in this PR.

@hungaborhorvath
Copy link
Contributor Author

@alex-konovalov Thank you for the advices, I applied all of them.

Replace DirectFactorsOfGroupKN by DirectFactorsOfGroupByKN
(in tst files, as well).
More use of <Ref ....> and <C> in grpnames.gd.
More appropriate use of <M></M> in grpnames.gd.
Apply <K></K> around true, false and fail.
@hungaborhorvath
Copy link
Contributor Author

hungaborhorvath commented Dec 21, 2016

@alex-konovalov Thank you, I have put <K></K> around true, false and fail.

@@ -24,8 +24,8 @@
##
## <Description>
## For normal subgroups <A>U</A> and <A>V</A> of <A>G</A>,
## IsTrivialNormalIntersection returns
## true if <A>U</A> and <A>V</A> intersect trivially, and false otherwise.
## <Ref Oper="IsTrivialNormalIntersection"/> returns <K>true</K> if
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I am sorry for the conradictory message, but this actually seems a bit odd -- it's kind of recursive: a function's description linking to itself... :-). Do we do this elsewhere? I'd hope we do not... How about instead rephrasing this as "This operation returns ..." ? Or keep the name, but in a or tag... ? I wonder if there are other examples of functions/operations referencing themselves in the manual, and how they solved this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StructureDescription had this already, but somehow, never together with a link. Maybe because it referenced a function instead of an operation? I am not sure. Maybe @alex-konovalov can shed some light on how to do this properly.

Copy link
Member

@olexandr-konovalov olexandr-konovalov Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "self-referencing" is a valid question - I've also asked it at some point. It happens that GAPDoc renders references in a clever way, understanding when they refer to the same mansection or to another one. Compare, for example,

##  <ManSection>
##  <Var Name="GaussianIntegers"/>
##
##  <Description>
##  <Ref Var="GaussianIntegers"/> is the ring <M>&ZZ;[\sqrt{{-1}}]</M>
##  of Gaussian integers.
##  This is a subring of the cyclotomic field
##  <Ref Func="GaussianRationals"/>.
##  </Description>
##  </ManSection>

with its documentation here.

The rationale behind this is (as I see it - maybe @frankluebeck had other considerations in mind while developing GAPDoc) as follows. Using just IsTrivialNormalIntersection is incorrect: one should use GAPDoc markup at least to distinguish it by using different font with <C>IsTrivialNormalIntersection</C>. But:

  • it may happen that the text will be moved to another place in the manual later, or copied and pasted as a template for another GAPDoc section. Then one should take care and replace <C> by <Ref> to get a proper cross-reference. Using <Ref> from the very start allows to avoid such hassles in the future.
  • one could of course argue that moving <C>IsTrivialNormalIntersection</C> to another location will not cause any problems, because it will be rendered successfully anyway. But then there will be no cross-reference to a documented thing - the quality of the documentation will suffer. It's a good practice to put cross-references to everything which is documented. If you try to cross-ref undocumented thing, you will see the warning. When you use just <C> elements, you will never be warned, while it's good to care whether you mention documented or undocumented things.
  • finally, this will guarantee the correct behaviour if some future version of GAPDoc (or just rebuilding the manual with another settings) will change the style of these elements so that <C> and <Ref> will be displayed differently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. In that case I have no objections.

@fingolfin
Copy link
Member

@alex-konovalov This PR is blocked by your review requesting changes. Could you please either change this to approval/neutral, or else clarify which issues still need to be addressed? Thanks!

@olexandr-konovalov
Copy link
Member

@fingolfin thanks for reminding - done!

@olexandr-konovalov
Copy link
Member

@fingolfin do we agree to merge this then?

@fingolfin
Copy link
Member

The content seems OK. The only thing I am not completely happy with is the somewhat random asortment of commits - ideally, I prefer if commits are self-contained units, so that one can review a PR commit by commit. This probably was the case here originally, but isn't anymore with e.g. the last commit "Apply Alex Konovalov's advices".

So if @hungaborhorvath was willing to improve that, I'd love it. But I'll also survive it is merged as is (but in general, I'd like for us to improve the quality of our commits ;-)

@fingolfin
Copy link
Member

Also, lest I sound ungrateful: @hungaborhorvath thank you for your contributions, I really appreciate them, and hope my constant change requests are not too tiresome...

@hungaborhorvath
Copy link
Contributor Author

@fingolfin Could you please be more specific on what you are proposing? Should I somehow split the last two commits into several by some topic, or should I squash them? Or something else?

I tried to explain what happens where in the more detailed commit messages, and tried to write the first comment of this PR so that it could be used for merge comment.

@fingolfin
Copy link
Member

@hungaborhorvath Just looked again at this PR, and it's really only the last commit "Apply Alex Konovalov's advices " which I don't like -- I'd prefer if it was split and the changes in it were incorporated into the commits they affect. But anyway, this is a super nitpick.

So let's just merge this.

@fingolfin fingolfin merged commit fd9d4e3 into gap-system:master Jan 6, 2017
@hungaborhorvath hungaborhorvath deleted the SemidirectDecompositionsFix branch January 6, 2017 14:09
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants