Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6409 +/- ##
==========================================
- Coverage 79.03% 78.95% -0.08%
==========================================
Files 685 685
Lines 293517 294316 +799
Branches 8664 8664
==========================================
+ Hits 231987 232390 +403
- Misses 59729 60122 +393
- Partials 1801 1804 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
stertooy
left a comment
stertooy
left a comment
There was a problem hiding this comment.
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
PreImagesSetNC( Intersection( Range( f ), U ) )
|
@stertooy It's so good to get feedback on this work - I will get on with making the changes you suggest. |
|
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 []. |
|
Perhaps this is why I returned [] in PreImagesElm when the test failed: |
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:
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.
I think we should still keep Perhaps @fingolfin @ThomasBreuer @hulpke want to weigh in, given that they were active in #4809 and #5173? |
|
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 |
|
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). |
|
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: ***@***.***>
|
|
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: ***@***.***>
|
Teach args helper about multi-arg variadic functions.
…s were not supported (#6401) Add runtime argument checks to the user-facing GF2 and 8-bit kernel entry points instead of relying on debug-only assertions or unchecked representation assumptions. Use shared Require helpers for compressed vectors and matrices, and tighten plain-list validation where these entry points expect list-backed inputs. Harden the exported vec8bit kernel entry points that assumed a first row existed, and fix SHIFT_VEC8BIT_RIGHT for empty compressed vectors. AI assistance from OpenAI Codex helped prepare these kernel validation changes. Co-authored-by: Codex <codex@openai.com>
Resolves #2570 Co-authored-by: Codex <codex@openai.com>
If input group is infinite or not solvable, fail is returned.
Mostly avoid putting to many things on one line, for better readability and more accurate code coverage tracking. Also remove obsolete restrictions for perfect group orders 86016, 368640, 737280
... as discussed on PR #6357
This undocumented function accepted more than one argument, but ignored any beyond the first. Apparently this was introduced in the GAP 4.5 days for compatibility reasons? But compatibility with what? No package known to me calls this. For reference in GAP 4.4 it had two arguments (but also was undocumented, and not called by any outside code).
The OrbitStabilizer entry in the reference manual only documented the algorithm, not what the operation returns. Adds: - A short sentence explicitly stating the return value is an immutable record with `orbit` (the orbit as a list) and `stabilizer` (the point stabilizer subgroup of G). - A small example block using the same `g:=Group((1,3,2),(2,4,3));;` setup already used by the adjacent `Stabilizer` example, so the rendered output is identical to verified output already in this file.
Also remove some now redundant methods.
Before:
gap> m:=RandomMat(256, 256, GF(1000000007));;
gap> mat:=Matrix(IsZmodnZMatrixRep, GF(1000000007), m);;
gap> RankMat(mat)=256; time;
true
170
gap> DeterminantMat(mat); time;
ZmodpZObj( 873487643, 1000000007 )
23982
After:
gap> m:=RandomMat(256, 256, GF(1000000007));;
gap> mat:=Matrix(IsZmodnZMatrixRep, GF(1000000007), m);;
gap> RankMat(mat)=256; time;
true
161
gap> DeterminantMat(mat); time;
ZmodpZObj( 873487643, 1000000007 )
1949
* Align and clarify break loop instructions further * Adjust <Log> blocks to new late error msg * Regenerate tst/testspecial
They are not referenced anywhere and nowadays at most of historical interest.
It does not seem to serve any purpose (anymore?)
There was an incorrect comment claiming that SyGAPCRC only ever returns 0 in case of an error. There was also code that looked as if it ensured that (by checking if `crc` is 0 and if so replacing it by 1). But this code in fact was a no-op, and has been one since 1997, when the code was changed to return `crc >> 4` instead of `crc` -- yet both `0 >> 4` and `1 >> 4` are equal to 0. Regarding the other changes: - `(crc >> 8) & 0x00FFFFFFL` is the same as `crc >> 8` since `crc` is an unsigned 32 bit value - removed a comment from the 1990ies suggesting that `SyGAPCRC` should ignore whitespace and comment changes; but we will never do that (as it would be very difficult, break backwards compatibility, and have little practical use) - align right shift in SyGAPCRC and FuncCrcString: one was "emulating" a signed shift, the other didn't. This didn't cause a problem in practice because essentially every compiler in active uses nowadays implements signed right shifts; but the C standard does not require this. Now both use the same branchless emulation code which e.g. clang compiles down into a single instruction (GCC, MSVC, ICC do not, but that's fine, this code is not a bottleneck for anything)
Bumps [actions/checkout](https://github.com/actions/checkout) from 6 to 7. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v6...v7) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Also treat readline headers as system includes. This avoids Homebrew readline header warnings under --enable-Werror. Co-authored-by: Max Horn <max@quendi.de> Co-authored-by: Codex <codex@openai.com>
In case someone uses '%g' in a format string but fails to provide a matching argument. Moreover, make sure we don't use `arg2` more then once.
There was a problem hiding this comment.
This file was added by accident and should be removed again.
ThomasBreuer
left a comment
ThomasBreuer
left a comment
There was a problem hiding this comment.
Up to now, PreImagesSomething( map, obj ) is defined as follows.
If obj is not an element (or subset, respectively) of Range( map ) then anything may happen.
If obj is an element (or subset, respectively) of Range( map ) but not of Image( map ) then fail is returned.
Otherwise, an element (or subset, respectively) of Source( map ) is returned that maps to obj under map.
The idea of the current pull request is to define functions PreImagesSomethingNC with the above definition, and to change the functions PreImagesSomething to signal an error if obj is not an element (or subset, respectively) of Range( map ), and to do the same as PreImagesSomethingNC otherwise.
Thus an error message is guaranteed for PreImagesSomething in the former case, and the user can decide to omit the membership test in Range( map ) by calling PreImagesSomethingNC.
(The text for the release notes would look like that if the pull request gets merged in this form.)
The proposed changes do not address the problems in the example from #4809 and in the second example from #4088, where one asks for the preimage of an element that is in the range but not in the image of the map.
My understanding of the discussion in #4088 was that the membership test for obj in the image of map is what one wants to skip in the new NC variants.
(If this is the case then it will be not difficult to change the pull request accordingly.)
The documentation of the functions should say that fail is returned if obj is not an element (or subset) of the range of map. Currently the proposed text says that the membership is tested.
(An alternative to a fail result would be an error. Of course, if we change the code such that membership in the image of map is tested, then a fail result is correct.)
| if not ( x in Range( f ) ) then | ||
| return fail; | ||
| fi; | ||
| return PreImagesRepresentative( f, x ); |
There was a problem hiding this comment.
| return PreImagesRepresentative( f, x ); | |
| return PreImagesRepresentativeNC( f, x ); |
(The old code should run into an infinite loop, which means that there is no test example for this method.)
| if not (elm in Range(hom)) then | ||
| return fail; | ||
| fi; | ||
| return PreImagesElm( hom, elm ); |
There was a problem hiding this comment.
| return PreImagesElm( hom, elm ); | |
| return PreImagesElmNC( hom, elm ); |
(Again, there seems to be no test example.)
| if not IsSubset( Range(hom), elms ) then | ||
| return fail; | ||
| fi; | ||
| return PreImagesSet( hom, elms ); |
There was a problem hiding this comment.
| return PreImagesSet( hom, elms ); | |
| return PreImagesSetNC( hom, elms ); |
| if not ( elm in Range(emb) ) then | ||
| return fail; | ||
| fi; | ||
| return PreImagesElm( emb, elm ); |
There was a problem hiding this comment.
| return PreImagesElm( emb, elm ); | |
| return PreImagesElmNC( emb, elm ); |
| ## <P/> | ||
| ## Anything may happen if <A>elm</A> is not an element of the range of | ||
| ## <A>map</A>. | ||
| ## From &GAP; version 4.16.0 |
There was a problem hiding this comment.
adjust to the GAP version number in question
| if not ( n in Range(rel) ) then | ||
| return fail; | ||
| fi; | ||
| return PreImagesElm( rel, n ); |
There was a problem hiding this comment.
| return PreImagesElm( rel, n ); | |
| return PreImagesElmNC( rel, n ); |
| ## From &GAP; version 4.16.0 | ||
| ## <C>PreImagesElm</C> was renamed <C>PreImagesElmNC</C> | ||
| ## throughout the library, and <C>PreImagesElm</C> now checks that | ||
| ## <A>elm</A> is an element of the range before calling <C>PreImagesElmNC</C>. |
There was a problem hiding this comment.
The definition should say what happens if this check fails.
| ## From &GAP; version 4.16.0 <C>PreImagesRepresentative</C> | ||
| ## was renamed <C>PreImagesRepresentativeNC</C> throughout the library, | ||
| ## and <C>PreImagesRepresentative</C> now checks that <A>elm</A> is an | ||
| ## element of the range before calling <C>PreImagesRepresentativeNC</C>. |
There was a problem hiding this comment.
The definition should say what happens if this check fails.
| ## From &GAP; version 4.16.0 <C>PreImagesSet</C> | ||
| ## has been renamed <C>PreImagesSetNC</C> throughout the library, | ||
| ## and <C>PreImagesSet</C> now checks that <A>elms</A> is a | ||
| ## subset of the range before calling <C>PreImagesSetNC</C>. |
There was a problem hiding this comment.
The definition should say what happens if this check fails.
There was a problem hiding this comment.
Many thanks for all these suggestions and making it clear how to proceed.
I have dealt with most of these suggestions, but there is more work to do.
Are we now agreed that, for the non-NC versions:
- elm/elms/coll not in the range => an Error is thrown, and
- elm/elms/coll in the range but not the image => fail is returned?
That being the case, I am adding error messages.
There was a problem hiding this comment.
Yes, this is how I understand the discussions in #4088 and #4809.
The "Error is thrown" part is changed behaviour in the sense that the current documentation promises nothing in this situation, hence the documentation should state that now an error is promised.
The "fail is returned" part is a bugfix in the sense that the documentation promises this but the current code does not follow this rule. (The examples in #4088 show that the current code may return a wrong element or run into an error.)
And the new NC variants allow one to skip the checks that are necessary to achieve these changes.
|
Advice required about PreImagesSet. If R is the range of map, and if the set U is a subset of R such that the intersection of U and the image of map is V then, mathematically, PreImagesSet(U) is PreImagesSet(V). Is that what should be implemented? At present, fail is returned. |
I see, The documentation of |
Is it not acceptable to have or perhaps, if Something like this means we keep the old definition of |
9 participants
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.