-
-
Notifications
You must be signed in to change notification settings - Fork 629
Make the is_isomorphic
method for AbelianGroup
and PermutationGroup
work for more groups
#40268
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
base: develop
Are you sure you want to change the base?
Make the is_isomorphic
method for AbelianGroup
and PermutationGroup
work for more groups
#40268
Conversation
is_isomorphic
method for AbelianGroup
and PermutationGroup
word for more groupsis_isomorphic
method for AbelianGroup
and PermutationGroup
work for more groups
sage: G.is_isomorphic(H) | ||
Traceback (most recent call last): | ||
... | ||
sage.libs.gap.util.GAPError: Error, cannot test isomorphism of infinite groups |
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.
I suggest catch it and re-raise NotImplementedError. Avoid exposing too much implementation detail, especially since we don't want upstream catch GAPError
.
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.
Good point. I did see some other parts of the library do throw a GAPError
, e.g. the PrimitiveGroups
function from permgroups_named.py
throws a GAPError
when called with too large a degree:
sage: PrimitiveGroups(2^12).cardinality()
Traceback (most recent call last):
...
GAPError: Error, Primitive groups of degree 4096 are not known!
But I see the utility of making the error message more user friendly. I could wrap the isomorphism check in a try-except block as follows:
try:
iso = left._libgap_().IsomorphismGroups(right)
except GAPError as e:
raise NotImplementedError(e) from None
This would catch any GAPError
and retype it as a NotImplementedError
hiding the original exception context. The only issue is that this would catch all GAPErrors
indiscriminately, which might cause weird and confusing error messages if there are GAPErrors
unrelated to isomorphisms checking of infinite groups. What are your thoughts on this?
if not isinstance(right, AbelianGroup_class): | ||
return False | ||
iso = left._libgap_().IsomorphismGroups(right) | ||
return str(iso) != "fail" |
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.
is there a way to check if an object is fail in gap itself instead of converting it to str?
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.
Yes, fail
is a boolean in gap so the equality check x = fail;
in gap would do this. I took the str(iso) != "fail"
code from the is_isomorphic
method of PermutationGroup
:
sage/src/sage/groups/perm_gps/permgroup.py
Line 4362 in 11baeed
return str(iso) != 'fail' |
Similar checking by conversion to string also appears to happen in:
sage/src/sage/libs/gap/libgap.pyx
Line 426 in 11baeed
if str(ret) == 'fail': |
sage/src/sage/interfaces/gap.py
Line 451 in 11baeed
if x == 'fail': |
Looking around in the codebase, I could avoid the conversion to string if I do iso == libgap.eval('fail')
instead. I can implement the change here + in the relevant places in PermutationGroup
. Do you think that would be preferable to doing a string conversion?
This PR fixes #39893, see also #39890. The crux of the issue is that the
G.is_isomorphic(H)
method ofAbelianGroup
returnsFalse
if the groupH
is not an instance ofAbelianGroup_class
, which caused quite confusing behavior in combination withpermutation_group
, whereG.is_isomorphic(G.permutation_group())
returnsFalse
, even thoughpermutation_group
in its documentation states that it "Return the permutation group isomorphic to this abelian group".This seems undesirable, so I've fixed it by making the method use the GAP group isomorphism method when both groups are not instances of
AbelianGroup
.Similarly, the
PermutationGroup
is_isomorphic
method seems to reject non-permutation group instances, and then uses the GAP isomorphism function anyway. Since the GAP isomorphism checking function can handle isomorphisms between permutation and non-permutation groups, I relaxed the constraint there, so nowG.permutation_group().is_isomorphic(G)
also returns true for anAbelianGroup
instanceG
.📝 Checklist
⌛ Dependencies
No PR dependencies