-
Notifications
You must be signed in to change notification settings - Fork 165
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
Minor fixes that could go into 4.10 as well #3508
Conversation
if source size is known and equals image size. This fixes gap-system#3431
The code assumed erraneously that the `IsomorphismFpGroup` of the radical factor is given on the free generators. That does not need to be the case (in which case weird errors happen). This fixes gap-system#3496.
@hulpke thanks. I've assigned this to 4.10.3 milestone - suggest to NOT to backport this to stable-4.10 until GAP 4.10.2 is out (expect to happen later this week). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, though the SetIsInjective
call could be made slightly more general (see change suggestions)
@@ -878,6 +878,9 @@ local g, sq, hom; | |||
g:=arg[1]; | |||
sq:=CallFuncList(SQ,arg); | |||
hom:=GroupHomomorphismByImages(g,sq.image,GeneratorsOfGroup(g),sq.imgs); | |||
if HasSize(g) and Size(g)=Size(sq.image) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if HasSize(g) and Size(g)=Size(sq.image) then | |
if HasSize(g) then |
@@ -878,6 +878,9 @@ local g, sq, hom; | |||
g:=arg[1]; | |||
sq:=CallFuncList(SQ,arg); | |||
hom:=GroupHomomorphismByImages(g,sq.image,GeneratorsOfGroup(g),sq.imgs); | |||
if HasSize(g) and Size(g)=Size(sq.image) then | |||
SetIsInjective(hom,true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SetIsInjective(hom,true); | |
SetIsInjective(hom, Size(g)=Size(sq.image)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not merge this now. I will approve this PR once 4.10.2 will be out. For now "requesting changes" just to block merging.
@alex-konovalov You can remove your requested changes. |
@wilfwilson done! |
@hulpke I set release notes: not needed" label then. |
These are fixes for (an issue related to #3431) and #3496.
The circumstances of the errors are complicated and there were error messages, not wrong results, that I do not think they of help to describe them explicitly in an update description beyond summarizing them as "numerous errors")
Both issues are in 4.10 code.
Fixes #3496