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 schur_multiplier #2965

Merged
merged 3 commits into from
Nov 1, 2023
Merged

add schur_multiplier #2965

merged 3 commits into from
Nov 1, 2023

Conversation

ThomasBreuer
Copy link
Member

@ThomasBreuer ThomasBreuer commented Oct 27, 2023

and abelian_invariants and abelian_invariants_multiplier

resolves #2962

and `abelian_invariants` and `abelian_invariants_multiplier`
@thofma
Copy link
Collaborator

thofma commented Oct 27, 2023

Thanks! Why not abelian_invariants_schur_multiplier (for consistency)? I have not strong opinion.

@codecov
Copy link

codecov bot commented Oct 28, 2023

Codecov Report

Merging #2965 (bab90e7) into master (103262f) will decrease coverage by 0.22%.
Report is 4 commits behind head on master.
The diff coverage is 91.17%.

@@            Coverage Diff             @@
##           master    #2965      +/-   ##
==========================================
- Coverage   80.36%   80.14%   -0.22%     
==========================================
  Files         470      472       +2     
  Lines       66662    67094     +432     
==========================================
+ Hits        53573    53775     +202     
- Misses      13089    13319     +230     
Files Coverage Δ
src/Groups/sub.jl 97.19% <100.00%> (+0.20%) ⬆️
src/Groups/GrpAb.jl 90.39% <86.36%> (-0.43%) ⬇️

... and 10 files with indirect coverage changes

@ThomasBreuer
Copy link
Member Author

@thofma I have adjusted the name.

@thofma
Copy link
Collaborator

thofma commented Oct 30, 2023

Thanks, looks good to me. Not sure if @fingolfin wants to have a look too?

@@ -372,6 +372,41 @@ end

is_pgroup_with_prime(G::GrpAbFinGen) = is_pgroup_with_prime(ZZRingElem, G)

function abelian_invariants_of_vector(::Type{T}, v::Vector) where T <: IntegerUnion
Copy link
Member

Choose a reason for hiding this comment

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

add a comment maybe that explains in 1-2 lines what this is about? My guess: v is supposed to be a vector of integers. And this function converts between the two different conventions for what "abelian invariants" are:

  1. elementary divisors, so prime powers or zero -- this is what GAP uses to represent the "abelian invariants" of a group
  2. invariant factors -- a list of integers $d_1 \mid d_2 \mid \dots \mid d_k$

I linked to Wikipedia here, but e.g. Encyclopedia of Mathematics agrees as does Bourbaki. Wikipedia also cites Lang's "Algebra" in support, but he actually only states an "Elementary Divisors Theorem", but does not say what an "elementary divisor" is -- and his theorem produces what above is called "invariant factors"

But not every does, e.g. Hecke has elementary_divisors return what I call "invariant factors", and I just read a paper by Frank Lübeck where he also used this convention.

So as usual, Math is inconsistent as hell... sigh

Anyway, back to this function: it seems to convert from format 2 to format 1 above, a comment should say this.

We now have multiple choices: use format 1 or format 2; and call the result by one or by the other name... Whatever we do, we should make explicit what we mean, given that the terms alone are ambiguous.

Anyway, I am fine with sticking to either naming convention; and also , as long as we make it clear in the documentation which we use (offering a helper function to convert between them of course also would be nice)
As far as I can tell, Magma has two functions, PrimaryAbelianInvariants and AbelianInvariants which return the first resp. the second kind of sequence.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed this math inconsistency a while ago, but I am happy to make our system consistent. It does not need to happen in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

O.k., I will add some comments as suggested, and leave it to the discussion of issue #2968 how to change names of functions if necessary.

- added a description of `abelian_invariants_of_vector`
- added `elementary_divisors_of_vector` (with description)

The function names were chosen compatibly with `Hecke.elementary_divisors`
and `GAP.Globals.AbelianInvariants`, cf. issue #2968.
@thofma thofma merged commit 877bd18 into oscar-system:master Nov 1, 2023
13 of 19 checks passed
@ThomasBreuer ThomasBreuer deleted the TB_schur_multiplier branch November 1, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request topic: groups
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schur multiplier for groups
3 participants