Remove Section 87.2-5 on "Avoiding multiplication of permutations" in the reference manual (the described functionality does not actually work)

[wucas]

The method for avoiding multiplication of permutations described in Section 87.2-5 does not work.
It seems that the code providing the data structure used (which is the rgtObj and lftObj as described in Section 87.2-5) is commented out. Thus the section should be removed.

[ChrisJefferson]

Thanks! Indeed this section of the documentation has been wrong for.. about 20 years or so!

[codecov]

Codecov Report

Merging #3101 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3101   +/-   ##
=======================================
  Coverage   83.56%   83.56%           
=======================================
  Files         686      686           
  Lines      336759   336759           
=======================================
  Hits       281401   281401           
  Misses      55358    55358

[olexandr-konovalov]

@wucas could you please point out to the location of the commented out code - may be those line could now be deleted completely too.

[ChrisJefferson]

I'd prefer the commentedd lines stay -- it is in the partition backtrack code where it is useful to be able to see the history.

[markuspf]

Just for reference the code is here:

gap/lib/stbcbckt.gi

Line 1458 in 72d2c36

obj := rec( lftObj := Pr[ 1 ],

and I agree with @ChrisJefferson that for the moment it is more useful that the commented code is there for understanding what the algorithm is supposed to do and to not confuse the "casual" reader more about the intention of the code.

[olexandr-konovalov]

Looking at the code (thanks for pointing out the exact location), I agree that throwing away the line # rgtObj := Pr[ 2 ], and so on may not be helpful since without seeing the commented out code the reader may be curious why Pr[2] is bound but unused, or not bound at all - this will help to figure out at least a partial explanation.

On the other hand, I don't think that leaving the code commented out without explaining why it is commented out, and then throwing away the documentation will help "understanding what the algorithm is supposed to do and to not confuse the "casual" reader more about the intention of the code".

I think a better way of doing this would be, while deleting the documentation, to add the comment to gap/lib/stbcbckt.gi explaining that these lines were formerly used by the algorithm to avoid multiplication of permutations, but they were commented out and their documentation has been withdrawn. Making this comment in the same commit which removes the manual section will make it easier to connect things together.

Alternatively, one could comment out the XML code in doc/ref/stbchain.xml without removing it, and also connect this change with a comment in gap/lib/stbcbckt.gi as suggested in the previous paragraph.

[olexandr-konovalov]

I suggest to add some comment to doc/ref/stbchain.xml to connect commented out source code and withdrawn documentation via modifying them in the same commit.

[wucas]

@alex-konovalov I get the intention of keeping a trace of this feature. But I'm not sure how such an explanation would fit in the documentation. It seems at bit out of place to suddenly explain something about code that was commented out years ago. For the casual reader i would argue that as long as the functionality is undocumented i wouldn't worry about it. I only stumbled over it because it was documented. Otherwise I'd never had the idea to use it in the first place.
Also I personally am not a big fan of commenting the section out in doc/ref/stbchain.xml just because it would leave the code in a bigger mess than I found it in.
If someone tries to understand the code in gap/lib/stbcbckt.gi I at least wouldn't look at the source code of the documentation. If it is about helping someone reading the code I think the best would be to explain it briefly in the code itself by adding a comment there.

[olexandr-konovalov]

@wucas agree - I think it will suffice if you will add to this PR a comment at gap/lib/stbcbckt.gi saying something like

# Here and below the code that refers to `rgtObj` was used to avoid multiplication
# of permutations. It has been commented out for a long time, but accidentally remained
# documented in `doc/ref/stbchain.xml` until its withdrawal in 2018.

[wucas]

@alex-konovalov thanks for the suggestion. I added the comment at the first instance of rgtObj in the partition backtrack function in gap/lib/stbcbckt.gi.

[olexandr-konovalov]

Fine with me thanks - but it's necessary to squash commits into one when this will be merged (or you can use interactive rebase with git rebase -i master, squash them yourself, and then force-push to your branch).

[olexandr-konovalov]

Made a final look at the removed XML - and the only example in that section used Listing element (since it does not have a runnable code), so it was excluded from tests all the time ... :-(

<Listing><![CDATA[	
return PartitionBacktrack( G,	
  [ g, g,	
  OnPoints,	
  c -> c!.lftObj = c!.rgtObj ],	
  false, R, [ Pi_0, g ], L, R );	
]]></Listing>