-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Renaming PreImages... functions as PreImages...NC : GAP PR#5073 #949
Comments
Thanks @cdwensley I’m happy to fix this thanks for the report! |
That's great. Tried three PRs today for various packages but suspect I'll hear nothing from the other two! This thing has dragged on far too long, which is entirely my fault, but a good push now might result in some progress. Maybe you can help with my query on Slack? |
This is clearly nowhere near the top of your TODO list, which is very understandable. So I will make a start on a PR - tell me if you want me to stop! |
But that's only if I can manage to compile Semigroups! I can get ./configure to work, but not make: This is on an iMac less than two years old, so GCC and clang (whatever they are) should be OK. |
I'll welcome a pr with these changes! You need to ./autogen.sh in the semi groups directory before running configure. |
Yes, that was done first. The situation today is that 5.2.0 compiles just fine on my system, but not the GitHub version.
|
PR #965 created : approval required to run the tests. |
PreImagesRepresetnative, PreImages, PreImagesSet and PreImagesElm can all return incorrect results when the element(s) supplied are not in the range of the map.
This situation has been discussed in GAP issue #4809.
To rectify the situation the plan is to have NC versions of these four operations and to add tests to the non-NC versions.
The procedure to be adopted is as follows.
(1) Rename the four operations by adding 'NC' to their names, and then declare the original operations as synonyms of these. PR #5073 addresses this.
(2) Ask package authors/maintainers to convert all their calls to these functions to the NC versions (unless there is good reason not to do so).
A clause added to init.g ensures the package works in older versions of GAP.
(3) Once all the packages have been updated, add tests to the non-NC versions of the operations.
No progress has been made on this since February, but now seems a good time to continue with stage (2).
No PR is offered to Semigroups at the moment because it is not clear what to do.
In attributes/homomorph.gi there are two methods implemented, and both of these apply tests, so should clearly not be converted to NC.
On the other hand, in attributes/isorms.gi there are two new methods implemented which do not do tests, so perhaps should be converted to NC.
Apart from these methods, there are just a few calls to PreImagesRepresentative.
How would the package authors like to proceed?
The text was updated successfully, but these errors were encountered: