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.

Sign up for GitHub

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

Jump to bottom

Fix declaration of sparse action homomorphisms. #3281

Merged
merged 3 commits into from
Feb 14, 2019
Merged

Fix declaration of sparse action homomorphisms. #3281

merged 3 commits into from
Feb 14, 2019

Conversation

hulpke
Copy link
Contributor

@hulpke hulpke commented Feb 12, 2019 •
edited
Loading

Sparse Action homomorphisms cannot use OrbitishFO but need to use their own creator function (as point seed is list of points and thus need to apply TestIdentityAction differently.)

This fixes #3279

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0007%) to 85.071% when pulling ef066c0 on hulpke:fixes into df547af on gap-system:master.

@coveralls
Copy link

coveralls commented Feb 12, 2019 •
edited
Loading

Coverage Status

Coverage decreased (-0.001%) to 85.071% when pulling da453d9 on hulpke:fixes into df547af on gap-system:master.

@hulpke
FIX: Sparse Action homomorphisms cannot use OrbitishFO
106f7f0 
but need to use their own creator function (as point seed is list of points and
thus need to apply `TestIdentityAction` differently.)

This fixes gap-system#3279
@codecov
Copy link

codecov bot commented Feb 12, 2019 •
edited
Loading

Codecov Report

Merging #3281 into master will decrease coverage by <.01%.
The diff coverage is 81.48%.

@@            Coverage Diff             @@
##           master    #3281      +/-   ##
==========================================
- Coverage   85.23%   85.23%   -0.01%     
==========================================
  Files         696      696              
  Lines      344043   344093      +50     
==========================================
+ Hits       293238   293278      +40     
- Misses      50805    50815      +10
Impacted Files Coverage Δ
lib/oprt.gd 95.42% <81.48%> (-2.12%) ⬇️

ThomasBreuer
ThomasBreuer approved these changes Feb 13, 2019
View reviewed changes
Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fix the bug, thank you very much.
I have added a few comments in order to shorten the code.
(I see the changes from this pull request as one step to finally getting rid of the FO approach for creating the wrapper functions.)

lib/oprt.gd Outdated Show resolved Hide resolved
lib/oprt.gd Outdated Show resolved Hide resolved
lib/oprt.gd Outdated Show resolved Hide resolved
lib/oprt.gd Outdated Show resolved Hide resolved
lib/oprt.gd Outdated Show resolved Hide resolved
@hulpke
Copy link
Contributor Author

hulpke commented Feb 14, 2019

Thanks @ThomasBreuer

I agree with your remark concerning the FO function. I think a (related) question is how much the "external set" option (which is not really used anywhere) needs to be supported. Maybe this is best moved into a sepaarte issue.

@ThomasBreuer ThomasBreuer merged commit 00899c6 into gap-system:master Feb 14, 2019
@olexandr-konovalov olexandr-konovalov added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes backport-to-4.10 labels Feb 19, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.10.1 milestone Feb 19, 2019
@olexandr-konovalov
Copy link
Member

I think this should this be backported to stable-4.10 and mentioned in release notes.

@olexandr-konovalov olexandr-konovalov mentioned this pull request Feb 19, 2019
Closed
@fingolfin
Copy link
Member

Backported to stable-4.10 in commits 7abb7eb, 151b4a3, 8fa6830

@fingolfin fingolfin modified the milestones: GAP 4.10.1, GAP 4.10.2 Jun 13, 2019
@olexandr-konovalov olexandr-konovalov added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Jun 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThomasBreuer ThomasBreuer ThomasBreuer approved these changes

Assignees
No one assigned
Labels
backport-to-4.10-DONE release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Milestone
GAP 4.10.2
Development

Successfully merging this pull request may close these issues.

problem with TestIdentityAction
5 participants
@hulpke @coveralls @olexandr-konovalov @fingolfin @ThomasBreuer