Skip to content
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

changing to NC versions of Preimages... #84

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cdwensley
Copy link
Contributor

PreImages, PreImagesElm, PreImagesSet and PreImagesRepresetnative, 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 that the package still works in older versions of GAP.
(3) Once all the packages have been updated, add tests to the non-NC versions of the operations.

This PR attempts to implement (2) for package polycyclic. Why was this PR not made earlier when most of the other packages have made changes? Probably because of a comment by @fingolfin in PR#5073 that a major bug had to be fixed.
This PR defines 3 NC versions in init.g (PreImagesElm is not used) and changes calls to NC versions in 10 library files. A second commit makes a similar adjustment to some of the tests - perhaps these should not have been changed?
Changes made in the unmerged polycyclic PR#48 to the method for PreImagesSet have been included here (was that wise?).

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Merging #84 (26dc1ee) into master (90fae57) will increase coverage by 0.00%.
The diff coverage is 71.42%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #84   +/-   ##
=======================================
  Coverage   56.29%   56.29%           
=======================================
  Files          87       87           
  Lines       13015    13016    +1     
=======================================
+ Hits         7327     7328    +1     
  Misses       5688     5688           
Files Coverage Δ
gap/basic/convert.gi 76.70% <100.00%> (ø)
gap/basic/grphoms.gi 94.17% <100.00%> (+0.02%) ⬆️
gap/exam/generic.gi 82.60% <100.00%> (ø)
gap/pcpgrp/centcon.gi 40.52% <100.00%> (ø)
gap/pcpgrp/pcpattr.gi 31.03% <100.00%> (ø)
gap/pcpgrp/tensor.gi 93.41% <100.00%> (ø)
gap/cover/const/aut.gi 7.14% <0.00%> (ø)
gap/pcpgrp/general.gi 25.22% <0.00%> (ø)
gap/pcpgrp/schur.gi 73.56% <0.00%> (ø)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant