Skip to content

Don't use fast method for composing with identity mapping if source & range don't match #2533

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

Merged
fingolfin merged 1 commit into from
Jun 12, 2018
Merged

Don't use fast method for composing with identity mapping if source & range don't match #2533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions lib/mapprep.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1387,9 +1387,7 @@ InstallMethod( CompositionMapping2,
[ IsGeneralMapping and IsOne, IsGeneralMapping ],
SUM_FLAGS + 1, # should be higher than the rank for a zero mapping
function( id, map )
if not IsSubset(Source(id),Range(map))
and not IsSubset(Source(id),ImagesSource(map))
then
if not IsSubset(Source(id),Range(map)) then
# if the identity is defined on something smaller, we need to take a
# true `CompositionMapping'.
TryNextMethod();
Expand Down
13 changes: 13 additions & 0 deletions tst/testbugfix/2018-06-11-mapping.tst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# There was a bug where composition of an identity mapping and another
# mapping x where the Source of the identity mapping contained the
# ImagesSource of x, but not its whole range, simply returned x,
# resulting in a composition whose Range was strictly bigger than that
# of its first argument, and causing problems in IsomorphismPermGroup
#
gap> g := SymmetricGroup(7);;
gap> phi := GroupGeneralMappingByImages(g,g,[(1,2,3,4,5),(1,2)],[(1,2,3,4,6),(1,2)]);
[ (1,2,3,4,5), (1,2) ] -> [ (1,2,3,4,6), (1,2) ]
gap> psi := IdentityMapping(SymmetricGroup(6));
IdentityMapping( Sym( [ 1 .. 6 ] ) )
gap> Range(CompositionMapping(psi,phi));
Sym( [ 1 .. 6 ] )
Copy link
Member

@fingolfin fingolfin Jun 11, 2018

Choose a reason for hiding this comment

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

Hmm, phi has Sym(7) as its range (but not as its image!), so on a purely formal level, it shouldn't compose with psi like that, should it?

But OK, GAP doesn't seem to specify what it allows for composition, so I'll assume that only the image is relevant for the legality of the composition, not the range. I guess being stricter about this might be annoying in some situations, while the added "type safety" it provides may not be worth it. (Also, changing existing behavior like that of course is risky)

Copy link
Contributor Author

@stevelinton stevelinton Jun 11, 2018

Choose a reason for hiding this comment

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

@fingolfin -- the manual is rather silent on what is allowed for the sources and ranges in a composition, whether there is an implicit restriction, and so on. I'll flag an issue to document and test some set of rules, but I don't want it to get tangled in this, which seems to be a safe bugfix.

Copy link
Member

@fingolfin fingolfin Jun 11, 2018

Choose a reason for hiding this comment

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

Thanks, that's fine by me. So I'll just wait for the typos in the comments above to be fixed.