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 65.40931% with 431 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.97%. Comparing base (676342d) to head (2ad5aa9).

Files with missing lines	Patch %	Lines
lib/mapprep.gi	61.18%	85 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 32 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6409      +/-   ##
==========================================
- Coverage   79.02%   78.97%   -0.05%     
==========================================
  Files         685      685              
  Lines      294324   295121     +797     
  Branches     8647     8670      +23     
==========================================
+ Hits       232591   233076     +485     
- Misses      59925    60238     +313     
+ Partials     1808     1807       -1     

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

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.

[ThomasBreuer]

Thanks a lot for the changes, and sorry for not checking them earlier.
I have added a few comments which should be easy to address.

The only missing bit is then the remark for the release notes.
I think that this pull request can be described as follows.

• Fix the behaviour of the functions in question (in the sense of the examples from #4809).
• Introduce NC variants for the functions such that the newly introduced checks can be omitted in situations where they are unnecessary.
• Change the GAP library such that these new NC variants are called. (For GAP package maintainers, we should give a hint that the code of their package may be slowed down by the new checks, and that the recommended solution is to use the NC variants where appropriate.)

Once this is done, we should really merge this pull request.

(And afterwards we should think about follow-up changes for ImagesRepresentativeNC and related functions, since now we know which changes are needed.)

[ThomasBreuer]

I think the fail result is correct.
When PreImagesRepresentativeNC was called via a PreImagesRepresentative call then pres cannot be empty, but if one has called PreImagesRepresentativeNC directly then the statement "anything may happen if elm is not an element of the range of map" from the documentation does not cover all cases where elm is not an element of the image of map.

In this sense, we can either change the documentation of PreImagesRepresentativeNC to "anything may happen if elm is not an element of the image of map", or the PreImagesRepresentativeNC methods have to deal with the situation that elm is not in the image.
I prefer the former solution.

[ThomasBreuer]

Suggested change

	## <A>elm</A> is not an element of the range of <A>map</A>.
	## <A>elm</A> is not an element of the image of <A>map</A>.

PreImagesRepresentative checks whether elm is in the image, from this point of view PreImagesRepresentativeNC can assume that elm is in the image, and need not deal with situations where this is not the case.
(See the comment about line 1316 of mapping.gi.)

[ThomasBreuer]

Add a line

##  <Oper Name="PreImagesElmNC" Arg='map, elm'/>

below the corresponding line for PreImagesElm, because otherwise ?PreImagesElmNC will not work.

[fingolfin]

Thanks @cdwensley -- I really hope we can finally get this in for GAP 4.17.0

[fingolfin]

@cdwensley I took the liberty of fixing the merge conflicts with master, so if you want to do more work here, you should make sure to first pull from here.

[cdwensley]

@ThomasBreuer, @fingolfin : thanks for all these comments. Not sure when I shall be able to address them.
I have pulled as Max suggested - I was finding it very difficult to rebase from master.