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 64.70588% with 432 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.03%. Comparing base (4521b5b) to head (0ec5b91).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/mapprep.gi	58.45%	86 Missing ⚠️
lib/mapping.gi	62.74%	38 Missing ⚠️
lib/mapphomo.gi	71.87%	27 Missing ⚠️
lib/gprdperm.gi	48.97%	25 Missing ⚠️
lib/ghomperm.gi	62.50%	21 Missing ⚠️
lib/ghom.gi	58.13%	18 Missing ⚠️
lib/mgmring.gi	63.63%	16 Missing ⚠️
lib/alghom.gi	58.33%	15 Missing ⚠️
lib/relation.gi	78.57%	15 Missing ⚠️
lib/gprdmat.gi	57.57%	14 Missing ⚠️
... and 33 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6409      +/-   ##
==========================================
- Coverage   79.08%   79.03%   -0.05%     
==========================================
  Files         685      685              
  Lines      293698   294480     +782     
  Branches     8664     8641      -23     
==========================================
+ Hits       232262   232741     +479     
- Misses      59635    59936     +301     
- Partials     1801     1803       +2     

☔ View full report in Codecov by Harness.
📢 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 ) )

[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]

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?

[fingolfin]

Thanks for working on this again, @cdwensley. Unfortunately I don't have the bandwidth available to read all here, think about it, and come up with a good stance right now -- we'll get there, but I am afraid not in time for GAP 4.16.0 -- but I am confident it'll get merged eventually, certainly before 4.17.0.

I'll try to get back to this when I have some mental capacity for it

[hulpke]

I think part of the reason of NC functions is to actually not have to worry about issues such as membership (which might require an extra test).
I.e. the result of PreImagesSetNC should undefined if the set is not in the image of the map, but if it is returns the same as PreImagesSet.

[cdwensley]

Never expected it to make 4.16.0 but I will try to keep it up-to-date with gapdev.On 28 May 2026, at 01:03, Max Horn ***@***.***> wrote:fingolfin left a comment (gap-system/gap#6409) Thanks for working on this again, @cdwensley. Unfortunately I don't have the bandwidth available to read all here, think about it, and come up with a good stance right now -- we'll get there, but I am afraid not in time for GAP 4.16.0 -- but I am confident it'll get merged eventually, certainly before 4.17.0. I'll try to get back to this when I have some mental capacity for it —Reply to this email directly, view it on GitHub, or unsubscribe.Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today! You are receiving this because you were mentioned.Message ID: ***@***.***>

[cdwensley]

Agree - will make adjustments in due course.On 28 May 2026, at 02:49, Alexander Hulpke ***@***.***> wrote:hulpke left a comment (gap-system/gap#6409) I think part of the reason of NC functions is to actually not have to worry about issues such as membership (which might require an extra test). I.e. the result of PreImagesSetNC should undefined if the set is not in the image of the map, but if it is returns the same as PreImagesSet. —Reply to this email directly, view it on GitHub, or unsubscribe.Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today! You are receiving this because you were mentioned.Message ID: ***@***.***>

[cdwensley]

Many thanks @ThomasBreuer for all these corrections - sorry about the silly error.

[ThomasBreuer]

Thanks.
One more suggestion for fixing a typo.
And I am wondering whether also PreImageElm should be changed in the same spirit.

What is missing now is some text for the release notes. The following description would fit but is admittedly quite long.

• Change the definitions of PreImagesElm, PreImagesRepresentative, PreImagesSet in the case that the given element or set is not contained in the Range of the general mapping:
  Up to now, nothing was guaranteed in this case.
  From now on, an error message is shown in this case.
• Introduce new operations PreImagesElmNC, PreImagesRepresentativeNC, PreImagesSetNC,
  turn all GAP library methods for PreImagesElm, PreImagesRepresentative, PreImagesSet into methods for the new NC variants, and install methods for the non-NC variants that first test the given element or set for containment in the Range of the given general mapping and then delegate to the NC variants.
• In all GAP library methods for PreImagesElm, PreImagesRepresentative, PreImagesSet, check whether the given element or set is contained in the Image of the given general mapping.
  Up to now, some of the methods had assumed this and thus gave a wrong answer if the assumption was wrong.
• Users' code may benefit from changing calls to PreImagesElm, PreImagesRepresentative, PreImagesSet into calls of the NC variants where possible.

[cdwensley]

Regarding PreImageElm - this only applies to bijective maps, and I see no reason to change it.
Far more unsatisfactory are PreImage and PreImages. These two global functions just delegate to one of three operations, so why are they needed? Furthermore the only difference is between PreImage(map,elm) which calls PreImageElm(map,elm) and PreImages(map,elm) which calls PreImagesElm(map,elm) - the other two cases return the same sets.
Cannot remember why, but an NC version of PreImages has been added, but no PreImageNC. Surely it should be neither or both? I'm inclined to remove PreImagesNC. Advice needed!

[cdwensley]

Interestingly, PreImage is called around 150 times in the main library, but PreImages not at all.

[cdwensley]

How many of these should be changed? For example, in ghomperm.gi, line 308,
surely G := PreImage( hom );
should be: G := PreImagesRange( hom );
and (more complicated example with fp-groups) in grpfp.gi, line 948,
hom:=MaximalAbelianQuotient(G);
u:=PreImage(hom,TrivialSubgroup(Range(hom)));
could be:
u:=PreImagesSetNC(hom,TrivialSubgroup(Range(hom)));

[ThomasBreuer]

Regarding PreImageElm - this only applies to bijective maps, and I see no reason to change it.

The documentation of PreImageElm( map, elm ) says: Anything may happen if elm is not an element of the range of map.
Shouldn't this be changed to a promised error message, for the sake of consistency with the PreImagesSomething operations?
(This need not be part of this pull request.)

Far more unsatisfactory are PreImage and PreImages. These two global functions just delegate to one of three operations, so why are they needed?

The idea was that PreImages( map, obj ) and PreImage( map, obj ) promise an error message if obj is not an element or subset of Range( map ). I do not remember why two functions were introduced.

[cdwensley]

Thanks for your comments. Away for a few days - will address them when we return.

[fingolfin]

There was a merge conflict in tst/testinstall/ggmbi.tst‎ (looked like a copy&paste accident) which I just resolved (so you should git pull)

[cdwensley]

As suggested last week I have removed the recently introduced PreImagesNC from the library, though PreImages remains. In fact there were 12 calls to PreImages in the library which were changed to PreImagesNC and are now back to PreImages (in alglie.gi, grp.gi, grpnice.gi, grpprmcs.gi, mapprep.gi and relation.gi). Entries for PreImages in mapping.gd and mapping.gi have reverted to the versions in master.