Rename rank(A::GrpAbFinGen) to torsion_free_rank

[fingolfin]

Unfortunately the notion of 'rank' for groups clashes with that for abelian groups. The latter is also known as 'torsion free rank'. Hence I propose to rename rank for abelian groups here accordingly; in my experience (which obviously is colored by my background) there are much more places that use the former meaning of rank (minimal size of a generating set) than the latter ("torsion free rank").

The alternative would be to rename rank for groups, but I am not aware of any synonyms, so it'd probably need to be something artificial (group_rank? size_of_minimal_generating_set?)

There are probably some callers that need to be adjusted in here and in Oscar, but I didn't bother with trying to find them, first I would like to clear if there is even a chance of us applying this change or not :-).

CC @ThomasBreuer @fieker @thofma

[simonbrandhorst]

Outside of group theory the torsion free rank is common.
For instance Serge Lang in his texbook Algebra writes on page 46:
"The rank of $A/A_{tor}$ is also called the rank of A ."
Then $rank(\mathbb{Z}\times \mathbb{Z}/2\mathbb{Z})=2$ will catch them by surprise, or worse lead to wrong results.

[thofma]

Since I really want rank to land in Oscar, I am also in favor of this. It is quite disrupting though. I will discuss it with C. from K.

[fingolfin]

Just talked with @fieker about this and other people in KL. Thoughts from Claus

• there should be two different names
• no strong opinion on which names (so the names I suggest here would be OK by him, but could also be other)

(If we use two names, ideally each docstring would also point to the other one to help users).

Another suggestion

julia> #rank(::Type{torsion_free_factor}, x::AbGroup)

julia> #rank(torsion_free_factor(x))

julia> #rank(torsion_free_factor,x))

[thofma]

Another option Claus mentioned a while ago was "remove rank for abelian groups".

[fingolfin]

I am not sure what "remove rank for abelian groups" means. Let rank return an error for abelian groups?

[ThomasBreuer]

If rank is defined for groups then we cannot make it undefined for those groups that are abelian.
Or shall "remove rank for abelian groups" mean that rank should be removed from Hecke.jl, such that it is left to Oscar.jl to deal with it (and its ambiguity)?

[fingolfin]

Maybe it is too late for 1.0, but I still would like to get this in eventually -- I just stumbled over this again when dealing with free groups and not being able to just do rank on one of them.

(of course I could implement it on free groups as both definitions agree there, but then I'd be in a situation where I'd have to reject most other groups with an error ... or maybe I could just reject abelian groups and say "this is ambiguous" or so... Ugh)

[thofma]

@doc raw"""
    rank(A::FinGenAbGroup) -> Int
Return the rank of $A$, that is, the size of a minimal generating set for $A$.
See also [`torsion_free_rank`](@ref).
"""
rank(A::FinGenAbGroup) = error("rank(::FinGenAbGroup) has been renamed to torsion_free_rank")

Hm, should it error or return the minimal generating set size as promised by the docstring?

[fingolfin]

I am happy to activate the second method. The one concern I have is that existing code may silently break if we change the meaning of rank immediately. With this error I was hoping to catch at least errors in the Hecke and OscarCI test suites cased by this. We can of course change it before merging.

[lgoettgens]

It already chatched some errors in Oscar. See https://github.com/thofma/Hecke.jl/actions/runs/8047209417/job/21976054087?pr=1224#step:7:322

ERROR: LoadError: LoadError: LoadError: rank(::FinGenAbGroup) has been renamed to torsion_free_rank
Stacktrace:
  [1] error(s::String)
    @ Base ./error.jl:33
  [2] rank(A::Hecke.FinGenAbGroup)
    @ Hecke ~/work/Hecke.jl/Hecke.jl/src/GrpAb/GrpAbFinGen.jl:575
  [3] is_z_graded
    @ ~/work/Hecke.jl/Hecke.jl/oscar-dev/Oscar/src/Rings/mpoly-graded.jl:248 [inlined]
  [4] is_standard_graded
    @ ~/work/Hecke.jl/Hecke.jl/oscar-dev/Oscar/src/Rings/mpoly-graded.jl:215 [inlined]
  [5] Oscar.ProjectiveScheme(S::Oscar.MPolyDecRing{Nemo.FqFieldElem, Nemo.FqMPolyRing}, I::Oscar.MPolyIdeal{Oscar.MPolyDecRingElem{Nemo.FqFieldElem, Nemo.FqMPolyRingElem}})
    @ Oscar ~/work/Hecke.jl/Hecke.jl/oscar-dev/Oscar/src/AlgebraicGeometry/Schemes/ProjectiveSchemes/Objects/Types.jl:60
...

All of this should thus probably go into a breaking Hecke release. As this is just re-exported in Oscar, that would be a breaking change of Oscar as well, e.g. it needs to go into 1.0 or wait for 2.0. And the CI verifies that it is indeed breaking

[fingolfin]

The error seems to be in is_z_graded which looks like this:

function is_z_graded(R::MPolyDecRing)
  is_graded(R) || return false
  A = grading_group(R)
  return ngens(A) == 1 && rank(A) == 1 && is_free(A)
end

Since is_free is also being checked, it actually doesn't matter which rank definition we use, as they give equal results in the free case.

That said, of course this is a breaking change as we change the behavior of the documented & exported function rank.

[fingolfin]

So how shall we proceed? I can turn rank into a "working" method (the implementation is already there, just commented out).

Or perhaps we first make a change in Oscar.jl to make just is_z_graded work (e.g. by doing something like

if Hecke does not provide torsion_free_rank then define torsion_free_rank(G::FinAbGroup) = rank(G)
and then using torsion_free_rank in is_z_graded. Then we can re-run the tests here, and possibly catch more places that need torsion_free_rank, repeat.

(Just grepping for rank in Oscar is not really feasible as there are so many other methods for rank on other kinds of objecect).

[thofma]

Yes, we can do that. But still the question is whether we have enough time to change rank(G::FinGenGroupAb) before 1.0?

[fingolfin]

My understanding is that we do have the time, but in the end the release managers should decide. I'll open a PR at Oscar.jl and explain / ask about it.

[thofma]

P.S.: I am actually surprised, that there are so few rank(::FinGenAbGroup) calls overall. The only ones are in the tests?! Very strange.

[thofma]

Rebased

[codecov]

Codecov Report

Merging #1224 (bf86fb2) into master (a116eb9) will increase coverage by 0.03%.
The diff coverage is 50.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1224      +/-   ##
==========================================
+ Coverage   70.81%   70.85%   +0.03%     
==========================================
  Files         351      351              
  Lines      111970   111973       +3     
==========================================
+ Hits        79287    79333      +46     
+ Misses      32683    32640      -43     

Files	Coverage Δ
src/GrpAb/GrpAbFinGen.jl	74.70% <50.00%> (-0.21%)	⬇️

... and 29 files with indirect coverage changes

[fingolfin]

Thank you @thofma for working on both PRs. Let's hope it passes now