Renaming PreImages... as PreImages...NC : GAP PR#5073

[cdwensley]

These functions are extensively used in rcwa, so raising an issue before attempting a PR.

PreImagesRepresentative, PreImages, PreImagesSet and PreImagesElm can all return incorrect results when the element(s) supplied are not in the range of the map. This situation has been discussed in GAP issue #4809.
To rectify the situation the plan is to have NC versions of these four operations and to add tests to the non-NC versions. The procedure to be adopted is as follows.
(1) Rename the four operations by adding 'NC' to their names, and then declare the original operations as synonyms of these. PR #5073 addresses this.
(2) Ask package authors/maintainers to convert all their calls to these functions to the NC versions (unless there is good reason not to do so).
A clause added to init.g ensures the package works in older versions of GAP.
(3) Once all the packages have been updated, add tests to the non-NC versions of the operations.

No progress has been made on this since February, but now seems a good time to continue with stage (2). This PR attempts to implement (2) for package rcwa which only uses 3 of these operations (not PreImages itself).
I guess that you would like all the functions to be renamed with the NC versions - please say if not.
Depending upon the reply, a PR will be attempted.
One particular puzzle - why is PreImagesSet declared in rcwamap.gd when it is an existing operation?

[Stefan-Kohl]

Yes, after (1) has been implemented, please go ahead with your pull request to switch to using the NC versions of these functions. -- Thanks a lot!
"Why is PreImagesSet declared in rcwamap.gd" -- this is only for old versions of GAP (prior to 4.10), and can be removed. It was needed for a reason I don't remember anymore.

[cdwensley]

Thanks - will get on the a PR a.s.a.p.

[cdwensley]

PR #27 has been created, addressing this issue.

[Stefan-Kohl]

@cdwensley I think the main reference in the manual should still be to the non-NC versions of the functions (would you be so kind to change this back?) -- and in many cases, methods for the NC- and non-NC-versions will be the same, as there are no further checks to be made (for example, checking membership / subset relationship in source and range of an rcwa mapping is trivial, and done in all applicable cases by method selection etc.).

[Stefan-Kohl]

@cdwensley Also, what I see from your pull request is that after introducing the NC versions in the main library, the methods won't be for the non-NC-versions anymore(?) -- As said, in many (probably BY FAR most) cases, adding further checks for the methods for the non-NC-versions won't make sense as everything is checked already by method selection and / or other very cheap checks. Initially, I assumed you really know what you are doing -- but looking further made me doubt so ... .

[cdwensley]

As to "what I am doing" - GAP PR #5073 was nothing to do with me, I just volunteered to implement stage 2 of the process, contacting the packages.
As I understand it, up until now the three PreImages..., operations have not done checks, so anyone making use of them is assumed to have done any checks in advance.
If a package adds a new method for PreImagesElm, say, then either that method makes a check, in which case PreImagesElm is the correct name, or it does no checks, in which case PreImagesElmNC should be used.
It seems I have been hasty in assuming that you wished to have NC added to all the new methods implemented in rcwa. In rcwamap.gi, for example, some methods start with "if not n in R then return fail; fi;" and so should not be NC, while others have no such checks and can be NC.
So I could leave some methods as NC and revert others - but that would not be the best thing to do. I believe that if there is a method that starts with a check then it should be replaced by two methods:
(1) a nonNC-method which does the check and then calls the NC-method;
(2) an NC-method - the original method, but without the check.
Does this make sense to you?

[Stefan-Kohl]

Well -- e.g. for R = Integers, the argument filter IsInt for n completely replaces the check "if not n in R then return fail; fi;" -- and there are other similar cases. So, things are not THAT simple. In fact, I think now most (maybe even all) of the methods should rather be non-NC -- but the performed tests are always computationally very cheap (so, having NC-methods in addition wouldn't allow saving any significant amount of runtime or memory).

[cdwensley]

If you prefer all of the methods to be non-NC then that simplifies matters considerably. We can just decide not to proceed with the PR and I can mark my list of packages to be "done" for rcwa.

[Stefan-Kohl]

@cdwensley Having looked a bit closer at the issue, this seems to me the best option now. -- The methods are doing all needed tests (although / since they are computationally trivial). Having separate NC-methods usually makes sense if and only if some of the checks take a non-negligible time and / or need a non-negligible amount of memory.

[cdwensley]

Agreed, so we can close this PR and the issue. I will report this on GAP PR #5073.

[cdwensley]

In GAP PR#5073 @fingolfin has commented "I don't understand the "conclusion" for rcwa: wouldn't new code that uses, say, PreImagesNC on a group homomorphism potentially run into an error when used with an RCWA homomorphism due there not being a PreImagesNC method, just one for PreImages? I think we'll have PreImages delegate to PreImagesNC but we can't also delegate in the other direction otherwise there'll be infinite recursion?" I do not pretend to understand this comment, but believe that it cannot do any harm to define the four NC versions in your init.g, even if they are never used. Accordingly I shall create a new PR.