renaming PreImages... functions as PreImages...NC#98
Merged
Conversation
Codecov Report
@@ Coverage Diff @@
## master #98 +/- ##
==========================================
- Coverage 83.57% 83.55% -0.02%
==========================================
Files 11 11
Lines 5193 5193
==========================================
- Hits 4340 4339 -1
- Misses 853 854 +1
|
Contributor
Author
|
We are hoping to get GAP PR#5073 merged soon, even if some packages have not been updated. |
fingolfin
approved these changes
Feb 12, 2024
Contributor
Author
|
Many of the PRs for adding the NC versions in various packages have a failing codecov/patch. `I've always assumed that this can be safely ignored, but maybe I'm wrong? |
Member
|
@cdwensley it is OK to ignore |
olexandr-konovalov
approved these changes
Feb 17, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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).
This PR attempts to implement (2) for package Wedderga. This package uses 3 of the 4 functions (not PreImagesSet) and implements new methods for two of them. This PR changes all occurrences to NC, assuming that that is what the authors would wish.