-
Notifications
You must be signed in to change notification settings - Fork 162
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
NC versions of PreImages, PreImagesSet, PreImagesElm and PreImagesRepresentative #5073
Open
cdwensley
wants to merge
22
commits into
master
Choose a base branch
from
preimrep
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
b9bf221
methods for PreImRep renamed PreImRepNC
cdwensley 3a75945
replaced PreImRep by PreImRepNC in files from a... to g...
cdwensley a2294b6
renamed PreImRep as PreImRepNC in files m... to t...
cdwensley c56cee6
PreImRep -> PreImRepNC in xml files
cdwensley 21f4f83
some PreImRep -> PreImRepNC in tests
cdwensley 7d7142d
cases where fail being returned is immediately checked
cdwensley 3b0f954
PreImRep now a synonym for PreImRepNC
cdwensley d2ec3bc
PreImages renamed PreImagesNC
cdwensley e254160
PreImagesSet renamed PreImagesSetNC
cdwensley 1e9b47a
PreImagesElm renamed PreImagesElmNC
cdwensley 40c40cf
no mention of NC version in the tutorial manual
cdwensley ea3f7ef
attempting to elininate conflict in grp.gi
cdwensley 52230cf
hopefully fixed tst/testextra/makeperfect.g
cdwensley 7547338
attempting to resolve conflicts in /lib files
cdwensley 9f58f56
~use latest fitfree.gd
cdwensley 6c98837
using latesxt version of 5 files
cdwensley 3f02134
updating ghomfp.gi
cdwensley 1c28e22
updating ctblgrp.gi
cdwensley 02fd88c
remove some trailing spaces
cdwensley 64ecd34
removed some trailing spaces
cdwensley f49da1a
fixing problems reported by LINT
cdwensley 6e43a58
Merge branch 'master' into preimrep
fingolfin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can just be left as
PreImages
, in general in documentation we don't encourage people to useNC
methods of functions. Of course at the moment it doesn't make any difference.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisJefferson Is this rule formulated somewhere in the documentation?
(If we do not want to encourage using
NC
variants in the documentation then this pull request should not change examples from the manual to usePreImagesRepresentativeNC
instead ofPreImagesRepresentative
.)I would agree that the
NC
variants are mainly intended to be used inside functions where it is clear from the context that the tests are not necessary. On the other hand, one can argue that the same holds for an interactive GAP session, for example, if one wants to compute a preimage of an element that was just constructed as an element of the range of the mapping in question.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThomasBreuer Looking around when defining function we often define the NC and non-NC version at the same time, but when we reference we often only reference one.
I do think if we are mentioning
PreImagesNC
we should say what the difference is -- I know someone could follow the links to PreImages and PreImagesNC, but it is a bit strange here to see both mentioned.In general it is probably worth thinking about if we want to use
PreImagesRepresentative
orPreImagesRepresentativeNC
in most examples. I am not sure in general how much slowerPreImagesRepresentative
is going to be, so which we should be encouraging as a default -- but I would hopePreImagesRepresentative
, because it is safer for users who are beginners.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisJefferson Thanks for your thoughts.
Currently I find one statement about
NC
variants in the Tutorial Manual, which talks aboutSub<something>
vs.Sub<something>NC
, and one statement aboutSub<struct>NC
in the Reference Manual Section "Constructing Subdomains"I think there should be a general statement about
Func
vs.FuncNC
(in the Tutorial as well as in the Reference Manual), saying that the latter omits some argument checks and that the documentation of the individual functions will explain which checks are omitted.Some example that illustrates the difference should be shown in the Tutorial: When one creates a subgroup of a group then calling
SubgroupNC
instead ofSubgroup
will avoid membership tests for the subgroup generators; in the case of a permutation group, this may mean that the computation of a stabilizer chain for the big group can be avoided, which would hopefully be not expensive; for a matrix group such as the Baby Monster, callingSubgroupNC
will be the only reasonable way to create a subgroup, since asking for membership will be in general hopeless.In the Reference Manual,
Func
andFuncNC
should be documented in the same ManSection, and then cross-references need to mention only the non-NC
variant.Concerning the use of
NC
variants in manual examples, I see two possible strategies. Either we use only the non-NC
variants, or we use theNC
variants in all those cases where the context admits this (perhaps with a textual comment why theNC
variant may be called in examples in the Tutorial and in those cases where this is not obvious).I would expect that the differences in runtime are negligible in manual examples, and if this is not the case for some example then this example deserves a comment about this fact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it is not a good idea to mention NC variants in the tutorial manual, and will make the necessary changes. The more general comments are very interesting but are beyond the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed NC references in doc/tut/group.xml and doc/tut/algvspc.xml. Had problems trying to push these - no idea why - so used --force. Hope that does not cause any problems.