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.

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 some missing hom and isomorphism methods #4471

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ThomasBreuer
Copy link
Member

  • hom from FinGenAbGroup to GAPGroup
  • isomorphism between FinGenAbGroup and GAPGroup (both directions)

resolves #4460

- `hom` from `FinGenAbGroup` to `GAPGroup`
- `isomorphism` between `FinGenAbGroup` and `GAPGroup` (both directions)
@ThomasBreuer ThomasBreuer added topic: groups release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Jan 16, 2025
@thofma
Copy link
Collaborator

thofma commented Jan 16, 2025

Can we for the sake of brevity in the documentation just use Group in the docstrings? I think replicating the signatures and/or using Union might be a bit distracting.

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.39%. Comparing base (d589f51) to head (bcc316f).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4471   +/-   ##
=======================================
  Coverage   84.39%   84.39%           
=======================================
  Files         668      668           
  Lines       88567    88576    +9     
=======================================
+ Hits        74746    74755    +9     
  Misses      13821    13821           
Files with missing lines Coverage Δ
src/Groups/homomorphisms.jl 91.93% <100.00%> (+0.19%) ⬆️

... and 4 files with indirect coverage changes

@ThomasBreuer
Copy link
Member Author

@thofma

Can we for the sake of brevity in the documentation just use `Group` in the docstrings?
I think replicating the signatures and/or using `Union` might be a bit distracting.

Yes. Do we have to support also hom and isomorphism from and to MultTableGroup?

@fingolfin
Copy link
Member

Do we have to support also hom and isomorphism from and to MultTableGroup?

Have to? Perhaps not. Should we? Yes please!

@@ -159,6 +159,19 @@ function hom(G::GAPGroup, A::FinGenAbGroup, imgs::Vector{FinGenAbGroupElem}; che
return hom(G, A, gens(G), imgs; check)
end

# Map `A::FinGenAbGroup` to `G::GAPGroup` by prescribing images.
# Return a composition `A -> B -> G` where `A -> B` is an isomorphism
# to a `GAPGroup`.
Copy link
Member

Choose a reason for hiding this comment

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

Question:

  • how efficient are these composition maps?
  • do the maps we produce here already support things like is_injective, is_surjective, kernel?
  • What about images and preimages of subgroups?

Copy link
Member Author

Choose a reason for hiding this comment

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

These composition maps do not support the abovementioned functions.
(One might have guessed this from the tests, which deliberately avoid to call these functions.)

I think the solution is to add generic methods for composition maps.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please open an issue or two to track the state of this? We definitely want to support these (and more) on the long run.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Some time ago I had the idea to have a type for morphisms to groups of type FinGenAbGrp. The reason being that things like is_surjective, kernel etc can be determined generically for this.

@ThomasBreuer
Copy link
Member Author

@fingolfin Concerning MultTableGroup:
We have already isomorphism(G::GAPGroup, H::MultTableGroup) and isomorphism(G::MultTableGroup, H::GAPGroup),
also is_isomorphic(G::GAPGroup, H::MultTableGroup) and is_isomorphic(G::MultTableGroup, H::GAPGroup).

I will add the missing hom(G::GAPGroup, H::MultTableGroup, ...) and hom(G::MultTableGroup, H::GAPGroup, ...).

@fingolfin fingolfin enabled auto-merge (squash) January 17, 2025 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: groups
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing hom and isomorphism methods between FinGenAbgroup and GAPGroup
3 participants