-
Notifications
You must be signed in to change notification settings - Fork 113
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
add isomorphism
#1243
add isomorphism
#1243
Conversation
This is a follow-up to #1182. - deprecate `isomorphic_fp_group(G::GAPGroup)`, `isomorphic_pc_group(G::GAPGroup)`, `isomorphic_perm_group(G::GAPGroup)` in favour of `isomorphism(::Type{FPGroup}, G::GAPGroup)` `isomorphism(::Type{PcGroup}, G::GAPGroup)` `isomorphism(::Type{PermGroup}, G::GAPGroup)` - deprecate also (the undocumented) `isomorphic_group(::Type{T}, G::GAPGroup)` (does not occur in tests) in favour of `isomorphism` - added `isomorphism(::Type{FPGroup}, A::GrpAbFinGen)` `isomorphism(::Type{PcGroup}, A::GrpAbFinGen)` `isomorphism(::Type{PermGroup}, A::GrpAbFinGen)` - moved `_get_iso_function` and `isomorphic_group` from `sub.jl` to `homomorphisms.jl` - added tests for new conversions - adjusted tests to use `isomorphism` instead of the deprecated versions - added (::Type{T})(G::GAPGroup)
the old versin is used - at least the Galois stuff has to be adjusted as well |
Return an isomorphism from `G` to a group of type `T`. | ||
An exception is thrown if no such isomorphism exists. | ||
""" | ||
function isomorphism(::Type{T}, G::GAPGroup) where T <: Union{FPGroup, PcGroup, PermGroup} |
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.
Should we perhaps cache these isomorphisms, i.e.
function isomorphism(::Type{T}, G::GAPGroup) where T <: Union{FPGroup, PcGroup, PermGroup} | |
@attr function isomorphism(::Type{T}, G::GAPGroup) where T <: Union{FPGroup, PcGroup, PermGroup} |
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.
Caching is a good idea, but isomorphism
takes two arguments, and @attr
can deal only with unary functions.
(From this point of view, the old isomorphic_fp_group
etc. were more suitable.)
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.
Yeah, we can't just use @attr
, but we can still "manually" implement that caching. Anyway, not in this PR.
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. @fieker I've adjusted the Galois groups code, too
@@ -167,8 +167,7 @@ preimage(f::GAPGroupHomomorphism{S, T}, H::T) where S <: GAPGroup where T <: GAP | |||
## Groups created by isomorphisms |
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 really don't like this section title. Though I also don't have a great alternative suggestion... perhaps just
## Groups created by isomorphisms | |
## Group isomorphisms |
This is a follow-up to #1182.
deprecate
isomorphic_fp_group(G::GAPGroup)
,isomorphic_pc_group(G::GAPGroup)
,isomorphic_perm_group(G::GAPGroup)
in favour of
isomorphism(::Type{FPGroup}, G::GAPGroup)
isomorphism(::Type{PcGroup}, G::GAPGroup)
isomorphism(::Type{PermGroup}, G::GAPGroup)
deprecate also (the undocumented)
isomorphic_group(::Type{T}, G::GAPGroup)
(does not occur in tests) in favour of
isomorphism
added
isomorphism(::Type{FPGroup}, A::GrpAbFinGen)
isomorphism(::Type{PcGroup}, A::GrpAbFinGen)
isomorphism(::Type{PermGroup}, A::GrpAbFinGen)
moved
_get_iso_function
andisomorphic_group
from
sub.jl
tohomomorphisms.jl
added tests for new conversions
adjusted tests to use
isomorphism
instead of the deprecated versionsadded (::Type{T})(G::GAPGroup)