NC versions of PreImages, PreImagesSet, PreImagesElm and PreImagesRepresentative - version 2

[cdwensley]

This aims to continue work on the process outlined in issue #4809, and started in PR #5073.
(The latter PR was so far out of sync with the master that rebasing has proved to be very difficult, hence this version.)
The operations PreImages, PreImagesSet, PreImagesElm and PreImagesRepresentative have all been renamed throughout the library by adding 'NC' to their names. New versions of the four operations have been introduced which just add a simple test and then call the NC versions.
Authors of packages which include a method for one of the four operations have been asked to adjust their packages to prepare for the change. The packages which have merged a PR, and made a release after that are: cryst, fr, orb, polycyclic, matgrp, qpa, rcwa, semigroups, utils and wedderga, while fga has merged a PR but not made a release.
The fining package has closed PR#28 without merging it - it installs another method for PreImagesSet.

[codecov]

Codecov Report

❌ Patch coverage is 68.47134% with 297 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.80%. Comparing base (e1ee3e9) to head (168d917).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/mapprep.gi	64.28%	65 Missing ⚠️
lib/mapping.gi	64.51%	33 Missing ⚠️
lib/gprdperm.gi	56.09%	18 Missing ⚠️
lib/relation.gi	53.57%	13 Missing ⚠️
lib/alghom.gi	66.66%	10 Missing ⚠️
lib/field.gi	50.00%	10 Missing ⚠️
lib/gpfpiso.gi	68.75%	10 Missing ⚠️
lib/mgmring.gi	72.22%	10 Missing ⚠️
lib/fldabnum.gi	66.66%	9 Missing ⚠️
lib/gprdmat.gi	66.66%	9 Missing ⚠️
... and 33 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6409      +/-   ##
==========================================
+ Coverage   78.74%   78.80%   +0.05%     
==========================================
  Files         685      685              
  Lines      293314   294048     +734     
  Branches     8646     8648       +2     
==========================================
+ Hits       230975   231711     +736     
- Misses      60520    60527       +7     
+ Partials     1819     1810       -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[stertooy]

Very happy to see this PR revived!

One thing I'm a bit wary of, is how PreImagesSet( f, U ) always returns [ ] if U is not a subset of Range( f ), even if they have non-empty intersection. Intuitively, I would expect this function to follow the standard(?) mathematical definition of $f^{-1}(U)$, so something like

PreImagesSetNC( Intersection( Range( f ), U ) )

[stertooy]

There's a mistake here. The argument u is a subgroup of Range(hom), but here we check if it it's an element instead. This should probably be

if not IsSubset( Range(hom), u ) then

like here?

This is the root cause of the failure in teststandard here. In particular, if CRISP is loaded, it installs a method for NormalSubgroups which relies on PreImagesSet.

[cdwensley]

@stertooy It's so good to get feedback on this work - I will get on with making the changes you suggest.
The way I have done it most of the time, when the new test fails, PreImagesRepresentative returns fail, but PreImagesSet and PreImagesElm (which expect a list) return []. Is that correct, or should this be fail also for PreImagesElm? With PreImagesSet I can see that Intersection( Range(f),U) should be used.

[cdwensley]

Now I am really confused. The manual states: "If elms is a subset of the range of the general mapping map then PreImagesSet returns the set of all preimages of elms under map." Sticking to that, PreImagesSet should return fail if U is not a subset of the range. Alternatively, we could ditch PreImagesSetNC, and redefine PreImagesSet to return the preimage of the intersection, which could be [].

[cdwensley]

Perhaps this is why I returned [] in PreImagesElm when the test failed:
Two existing methods for PreImagesElm in mapphomo.gi call PreImagesRepresentative,
and if that returns fail then [] is returned.
I'll do nothing now until advice is received.

[stertooy]

I think this should be

InstallMethod( PreImagesRepresentative,

i.e. with the NC removed?

[stertooy]

The manual states: "If elms is a subset of the range of the general mapping map then PreImagesSet returns the set of all preimages of elms under map." Sticking to that, PreImagesSet should return fail if U is not a subset of the range.

I think the question is what we want the non-NC functions to do for elements/sets not contained in the range. I see two options:

1. We limit the scope of these functions to elements/sets contained in the range. The only difference between NC and non-NC functions, is that the latter are guaranteed to return fail if the element/set is not contained in the image.

2. The non-NC functions behave like the usual mathematical definition of a preimage. So in this case PreImagesElm( f, x ) would return the empty set [ ] if x is not contained in the image of f, and PreImagesSet( f, U ) would return the same as PreImagesSetNC( f, Intersection( Image( f ), U ) ).

The former option keeps the NC and non-NC versions closer together, with the only difference being a extra check, which is what an NC version usually indicates in GAP. The latter option sticks closer to the standard mathematical definitions. Either option is fine with me (with a preference for option 2), but it should at least be consistent across all the affected functions.

Alternatively, we could ditch PreImagesSetNC, and redefine PreImagesSet to return the preimage of the intersection, which could be [].

I think we should still keep PreImagesNC in this case, since certain functions/methods may have prior knowledge of a set actually being contained in the image, and they can then use the NC version to avoid running a superfluous membership check or intersection calculation.

Perhaps @fingolfin @ThomasBreuer @hulpke want to weigh in, given that they were active in #4809 and #5173?