defining in init.g the four PreImages...NC operations

[cdwensley]

This PR refers to issue #26, just reopened, and replaces the closed PR #27. Perhaps you might be willing to reconsider the situation? The plan is to complete gap-system/gap#5073 at the GAP days in March. Only init.g is changed in this PR.

[codecov]

Codecov Report

❌ Patch coverage is 93.93939% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.58%. Comparing base (aefd20c) to head (ba83384).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/rcwagrp.gi	77.77%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #28   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files          47       47           
  Lines      366448   366448           
=======================================
  Hits       364912   364912           
  Misses       1536     1536           

Files with missing lines	Coverage Δ
lib/perlist.gi	92.98% <100.00%> (ø)
lib/rcwagrp.gd	100.00% <ø> (ø)
lib/rcwamap.gi	92.11% <100.00%> (ø)
lib/rcwagrp.gi	83.56% <77.77%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fingolfin]

I don't understand this PR -- if all it does is define the names PreImagesElmNC, it may as well be not applied.

Looking at #27, it apparently was closed because the PreImagesRepresentative methods in this package always perform some checks?

But changing these methods to PreImagesRepresentativeNC methods has a purpose beyond "speeding up" things: It is important that everything does this, otherwise one cannot write uniform code: If some parts of GAP start to support PreImagesRepresentativeNC and some parts don't, then I can't write generic code using PreImagesRepresentativeNC -- it would fail for rcwa groups.

But this PR here does not do that; well it does it in "old" GAP versions that don't have PreImagesRepresentativeNC, by making it there an alias for PreImagesRepresentative. But once GAP has a proper PreImagesRepresentativeNC this patch here will not do anything anymore.

[cdwensley]

Please advise me what to do about all this PreImages stuff! It all started with GAP issue #4809, three and a half years ago, and proceeded with GAP PR #5073. It was agreed that all packages which used PreImages... should define the NC versions in their init.g before any changes could be made to the main library. It is this preparatory stage which has stalled. You talk above about '"old" GAP versions that don't have PreImagesRepresentativeNC' but the current gapdev still does not have any NC versions, and perhaps never will? I'm willing to start thinking about this again - but is it worth it?

[fingolfin]

The plan was to introduce a new operation PreImagesRepresentativeNC which omits "some" checks (which exactly is implementation defined).

The problem this idea faced (and still faces) is that one can't start using PreImagesRepresentativeNC (and the others -- for simplicity I'll just take about that one) in general code as long as it is only implemented for some map types.

At the same time, one can not stat implementing PreImagesRepresentativeNC methods as long as this operation is not defined. (Also, anything that starts to define PreImagesRepresentativeNC naively would then be instantly incompatible with any older GAP version that do not yet have PreImagesRepresentativeNC, which is annoying for various reasons).

To get out of this chicken-and-egg problem, the idea was to convert all packages to provide PreImagesRepresentativeNC methods; but to add compatibility code which ensures that if PreImagesRepresentativeNC is not there (= old GAP), things still work. And the way to achieve that is to define PreImagesRepresentativeNC as an alias for PreImagesRepresentative in that case.

(Once all packages which define PreImagesRepresentative methods so far instead define PreImagesRepresentativeNC we can then move to "phase 2" and introduce PreImagesRepresentativeNC as a separate operation in GAP, with methods for PreImagesRepresentative that delegate to PreImagesRepresentativeNC. And then we can start looking into modifying packages to remove certain checks from their NC methods. But that's down the road.)

But this PR doesn't do that: It doesn't ensure a PreImagesRepresentativeNC method is always installed for arguments of IsRcwaMappingInStandardRep: indeed, the changes in this PR will do nothing anymore as soon as GAP actually defines PreImagesRepresentativeNC.

So the correct way to proceed would be to do as you did in your original PR, and rename that PreImagesRepresentative method to PreImagesRepresentativeNC

[cdwensley]

Sorry to be so slow on the uptake with this, but I have been trying to get my head around it.
I think you are suggesting that this PR be closed and PR#27 reopened?
Then methods in RCWA are renamed as NC, even though they do provide checks.
It would, of course, be possible to split a method in two, with the checking in the non-NC part.
Is this what you advise?
I guess your comments here also apply to QPA, where in issue #78 it was concluded that there was "nothing to do".

[cdwensley]

So I have not reopened #PR27. Instead the changes made there have been repeated for PR#28.
(I have also managed to recreate my local preimages branch of gapdev.)
So now the NC versions are used throughout this package.
Is it wise to use NC in tests and manual examples?
Where to go from here?

[fingolfin]

The history of this PR was messed up, so I've rebased it now to make the diff readable.

[fingolfin]

The documentation should not reference the NC versions (except possibly to plain the difference, but I don't think that's necessary here, given that that there currently is none)

[fingolfin]

I've applied some more changes and this would be OK from my POV now.

Would be great if @Stefan-Kohl could merge this and the other PR and make a release ; or alternatively gives me permission to do so on his behalf.

[Stefan-Kohl]

@fingolfin Yes, please merge this (assuming the tests run through without diffs), and make a release. - Thanks a lot!