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

renaming methods in homwdata.gi to PreImages...NC #67

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

cdwensley
Copy link
Contributor

PreImages, PreImagesElm, PreImagesSet and PreImagesRepresentative, 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 methods for 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 orb which has one method for PreImagesElm and two methods for PreImagesRepresentative in the file homwdata.gi. These methods do no tests so should be NC versions.
These methods are apparently not called anywhere in the orb library or in any of the tests, so maybe this change is unnecessary?

This file also has methods for ImagesElm and ImagesRepresentative. It has been suggested that these should also have NC versions as part of PR#5073, but no decision has been made to date. Any comment?

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.77%. Comparing base (d7d2cd0) to head (006a861).
Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
+ Coverage   42.08%   44.77%   +2.68%     
==========================================
  Files          16       16              
  Lines        5978     6187     +209     
==========================================
+ Hits         2516     2770     +254     
+ Misses       3462     3417      -45     
Files with missing lines Coverage Δ
gap/homwdata.gi 65.21% <100.00%> (+7.32%) ⬆️

... and 8 files with indirect coverage changes

@fingolfin fingolfin merged commit d339de3 into gap-packages:master Jan 3, 2025
11 of 14 checks passed
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.

2 participants