Document LowIndexSubgroups, add LowLayerSubgroups, some fixes (partial rebase of #2035) #2158
Conversation
fingolfin
added
kind: bug
fingolfin
requested review from
ChrisJefferson,
hulpke,
markuspf and
olexandr-konovalov
Codecov Report
@@ Coverage Diff @@
## master #2158 +/- ##
==========================================
+ Coverage 69.7% 70.02% +0.31%
==========================================
Files 482 482
Lines 254592 257653 +3061
==========================================
+ Hits 177466 180411 +2945
- Misses 77126 77242 +116
|
hulpke
left a comment
hulpke
left a comment
There was a problem hiding this comment.
I'm happy with doing this separately -- my PR is really just for me to indicate what should go in 4.9
lib/grp.gd
Outdated
| ## | ||
| ## <Description> | ||
| ## These operations computes representatives of the conjugacy classes of | ||
| ## The operation <C>LowIndexSubgroups</C> computes representatives of the |
There was a problem hiding this comment.
Actually GAPDoc recommends to use <Ref> inside <ManSection> even if it refers to the object documented in this ManSection: see example here:
GAPDoc will not expand the reference in this case. IIRC this is useful because if you copy and paste the text elsewhere, <Ref> will be there already, and also in case the manual layout will display <Ref> and <C> elements differently. Perhaps there are actually deeper reasons, which @frankluebeck may tell.
Same remark refers to LowLayerSubgroups below.
Yes, an output format could decide different layout for referenced functions/operations/... and for general code. |
|
So I can adjust the GAPDoc comments of course, no problem, just should warn that @hulpke will then have to deal with it when rebasing his PR on master -- though it shouldn't be a big issue, one of his commits will cause a conflict, but can then be skipped (via |
fingolfin
force-pushed
the
mh/hulpke-for-master
branch
from
120b0fa to
8dcfd26
Compare
lib/grp.gd
Outdated
| ## <Oper Name="LowIndexSubgroups" | ||
| ## Arg='G, index'/> | ||
| ## <Oper Name="LowIndexSubgroups" Arg='G, index'/> | ||
| ## <Oper Name="LowLayerSubgroups" Arg='G, layer'/> |
There was a problem hiding this comment.
While looking into tweaking the GAPDoc, I realized that LowLayerSubgroups actually is a global function, not an operation. Moreover, it takes additional (optional) arguments, and already has an documentation comment in grplatt.gd (which also is in the manual).
Perhaps the plan was/is to turn LowLayerSubgroups into an operation? In any case, the change as it is in this PR right now does not seem correct... @hulpke ?
There was a problem hiding this comment.
Yes, this probably should just be replaced by a reference to LowLayerSubgroup (or the REF-syntax...). Do you want me to send you a replacement draft?
fingolfin
force-pushed
the
mh/hulpke-for-master
branch
from
8dcfd26 to
9352b3b
Compare
as requested in gap-system#2131 Also added `LowLayerSubgroups` and minor rephrasing. Also redid part of gap-system#683, as for some reason (probably my stupidity) part of gap-system#683 had fallen out of master. Added it again.
|
@hulpke I now updated the ref to |
|
@fingolfin Yes, its fine (and I think I know enough git to deal with the superfluous bits in the other PR as well once this is merged.) |
fingolfin
added
the
release notes: added
5 participants
This is a rebased version of some changes in PR #2035. For an explanation, see there. My hope is that we can merge this ASAP into
master, then immediately after cherry-pick it into master.The changes in this branch should be cherry-picked into stable-4.9.