Adjust to renaming of `rank(A::FinGenAbGroup)` to `torsion_free_rank(A::FinGenAbGroup)` #3457

fingolfin · 2024-02-28T16:27:19Z

The renaming would be done on the Hecke side thofma/Hecke.jl#1224.

This is a breaking change, as code that currently uses rank(A::FinGenAbGroup) would either run into an error (current state of that PR), or even get "wrong" (well, "unexpected" perhaps) results.

This PR tries to get ahead of it by changing all uses of rank that really want a "torsion free rank" to use that. (Actually: in all those places the groups in question are free, and then both "ranks" coincide; so this change here is not so much about making sure the code stays correct as about allowing us to ensure via OscarCI test that we caught all places that call rank(A::FinGenAbGroup) (or at least all that are actually being tested...).

The big question then is this: I think we have more or less consensus that we want this change, at least eventually. The big question is: can we still get it into 1.0?

I am not aware of any consequences of this PR for the book (which of course is not saying there are none...), but since it is a really very breaking change, if we don't have it in 1.0 we'd have to wait for 2.0. And if we do that, then perhaps we shouldn't merge this here either before we have a clear time line for 2.0, otherwise the difference between master and 1.x is too painful.

So while I'd love this PR and the one in Hecke to land and be backported to 1.0, ultimately the release managers (@benlorenz @aaruni96) should decide. (I've added the backports label as a kind of request, of course it can and should be removed if it is decided that it is not suitable)

fingolfin · 2024-02-28T16:30:39Z

(I am also going to run the test suite on my computer to find more issues and will push updates here as I encounter and fix them)

fingolfin · 2024-02-28T16:32:20Z

As it turns out, the toric varieties code uses rank on abelian groups extensively, e.g. rank(picard_group(x)) and rank(torusinvariant_cartier_divisor_group) and more.

@HereAround are these groups generally torsion free (so the two kinds of rank coincide), or not?

... to torsion_free_rank(A::FinGenAbGroup), see also <thofma/Hecke.jl#1224>.

codecov · 2024-02-29T01:54:12Z

Codecov Report

Merging #3457 (f83e591) into master (556be5a) will increase coverage by 0.03%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3457      +/-   ##
==========================================
+ Coverage   81.91%   81.94%   +0.03%     
==========================================
  Files         562      563       +1     
  Lines       75522    75536      +14     
==========================================
+ Hits        61861    61897      +36     
+ Misses      13661    13639      -22

Files	Coverage Δ
experimental/FTheoryTools/src/auxiliary.jl	`94.87% <100.00%> (ø)`
.../ToricVarieties/NormalToricVarieties/attributes.jl	`98.61% <100.00%> (ø)`
.../ToricVarieties/NormalToricVarieties/properties.jl	`88.88% <100.00%> (ø)`
...metry/ToricVarieties/ToricDivisors/constructors.jl	`88.00% <100.00%> (ø)`
...ometry/ToricVarieties/ToricMorphisms/attributes.jl	`78.57% <100.00%> (ø)`
...etry/ToricVarieties/ToricMorphisms/constructors.jl	`67.85% <100.00%> (ø)`
...cVarieties/cohomCalg/VanishingSets/constructors.jl	`66.66% <100.00%> (ø)`
...raicGeometry/ToricVarieties/cohomCalg/auxiliary.jl	`97.14% <100.00%> (ø)`
src/Modules/ModulesGraded.jl	`73.42% <100.00%> (-0.03%)`	⬇️
src/Rings/mpoly-graded.jl	`90.70% <100.00%> (ø)`
... and 1 more

... and 1 file with indirect coverage changes

benlorenz · 2024-02-29T10:09:26Z

So while I'd love this PR and the one in Hecke to land and be backported to 1.0, ultimately the release managers (@benlorenz @aaruni96) should decide. (I've added the backports label as a kind of request, of course it can and should be removed if it is decided that it is not suitable)

I think it makes sense to backport this and we will try to do so but we need it really soon, what is the current approach (and timeline) for this? Check that this runs in the OscarCI for Hecke, do a breaking Hecke release, adapt the version and merge this PR?

I will try to run this (together with the Hecke PR) for the book examples now.

thofma · 2024-02-29T10:36:17Z

So while I'd love this PR and the one in Hecke to land and be backported to 1.0, ultimately the release managers (@benlorenz @aaruni96) should decide. (I've added the backports label as a kind of request, of course it can and should be removed if it is decided that it is not suitable)

I think it makes sense to backport this and we will try to do so but we need it really soon, what is the current approach (and timeline) for this? Check that this runs in the OscarCI for Hecke, do a breaking Hecke release, adapt the version and merge this PR?

Yes, that would be the plan. I could do a Hecke breaking release as soon as the OSCAR CI (with the matching branches) in thofma/Hecke.jl#1224 shows green.

Can you report back about the book examples as soon as you have the result?

benlorenz · 2024-02-29T11:47:27Z

Can you report back about the book examples as soon as you have the result?

No new errors in the booktests and no unexpected output changes, running with thofma/Hecke.jl@bf86fb2 (#1224), and on a branch (based on the backports branch) where I backported the first three commits on this PR.

lgoettgens · 2024-02-29T13:19:25Z

experimental/GModule/Cohomology.jl

@@ -474,7 +474,7 @@ export action, cohomology_group, extension, pc_group_with_isomorphism
 export induce, is_consistent, istwo_cocycle, all_extensions
 export split_extension, extension_with_abelian_kernel

-Oscar.dim(C::GModule) = rank(C.M)
+Oscar.dim(C::GModule) = torsion_free_rank(C.M)


Suggested change

Oscar.dim(C::GModule) = torsion_free_rank(C.M)

Oscar.dim(C::GModule) = rank(C.M)

This delegates to the module and not the group. So the change here is wrong

I'm not sure this is true. C.M is "the module", but there are some signatures with C::GModule{..., FinGenAbGroup} and the second type parameter is for C.M as far as I can see. Somebody who knows about the code should have a look.

The CI in Hecke agrees that here we have a "module". See https://github.com/thofma/Hecke.jl/actions/runs/8094631614/job/22124040318#step:7:1375

Oh, I think it can be both here. So we would need something like

Oscar.dim(C::GModule{..., FinGenAbGroup}) = torsion_free_rank(C.M) Oscar.dim(C:GModule) = rank(C.M)

Yes, exactly. This is an example from the tests:

julia> k, a = cyclotomic_field(5) (Cyclotomic field of order 5, z_5) julia> zk = maximal_order(k) Maximal order of Cyclotomic field of order 5 with basis AbsSimpleNumFieldElem[1, z_5, z_5^2, z_5^3] julia> U, mU = unit_group_fac_elem(zk) (Z/10 x Z, UnitGroup map of Factored elements over Cyclotomic field of order 5 ) julia> A, mA = automorphism_group(PermGroup, k) (Permutation group of degree 4, Map: permutation group of degree 4 -> generic group of order 4 with multiplication table -> set of automorphisms of Cyclotomic field of order 5) julia> C = gmodule(A, mU) G-module for A acting on U julia> typeof(C.M) FinGenAbGroup

joschmitt · 2024-02-29T13:42:28Z

experimental/GModule/GaloisCohomology.jl

@@ -1912,7 +1912,7 @@ function shrink(C::GModule{PermGroup, FinGenAbGroup}, attempts::Int = 10)
      o = Oscar.orbit(q, rand(gens(q.M)))
      if length(o) == order(group(q))
        s, ms = sub(q.M, collect(o), false)
-        if rank(s) == length(o)
+        if torsion_free_rank(s) == length(o)


This is another place where we need to be careful about the type of s I think.

If I remember the idea correctly, the change should be correct. But @fieker should confirm.

thofma · 2024-02-29T15:20:19Z

experimental/GModule/Cohomology.jl

@@ -474,7 +474,7 @@ export action, cohomology_group, extension, pc_group_with_isomorphism
 export induce, is_consistent, istwo_cocycle, all_extensions
 export split_extension, extension_with_abelian_kernel

-Oscar.dim(C::GModule) = torsion_free_rank(C.M)
+Oscar.dim(C::GModule) = rank(C.M)


There is no rank(::FinGenAbGroup anymore in conjunction with thofma/Hecke.jl#1224.

See the discussion above. C.M can also be a FreeModule (or something).

I find this dim function rather problematic to start with, there is a reason we use rank for modules in general... but that's besides the point right now

I'll push a fix

thofma · 2024-02-29T17:53:28Z

OK thofma/Hecke.jl#1224 is looking good. I'll tag 0.30.0 later today.

thofma · 2024-02-29T18:28:25Z

OK, later today means now.

thofma · 2024-02-29T19:05:11Z

Someone needs to approve and squash this.

fieker · 2024-02-29T19:31:28Z

As a comment: there are almost no requirements for the 2nd type. It needs to be a mathematical module, not a module type

…A::FinGenAbGroup)` (#3457) * Prepare for renaming of rank(A::FinGenAbGroup) ... to torsion_free_rank(A::FinGenAbGroup), see also <thofma/Hecke.jl#1224>. * Use is_z_graded in _regularity_bound * more rank -> torsion_free_rank * more * more more * Update experimental/GModule/Cohomology.jl * more more more * bump Hecke --------- Co-authored-by: Tommy Hofmann <[email protected]> Co-authored-by: Lars Göttgens <[email protected]> (cherry picked from commit 8b863ed)

- Add QQBar docs to the manual #3423 - do not show the OscarInterface banner #3422 - fix bugs in all_OD_infos #3419 - Ep/ Rename Spec to AffineScheme #3345 #3425 - Remove two mentions of Arb_jll #3431 - Tweak epimorphism_from_free_group #3430 - CI: re-enable nightly #3435 - support gen(G::GAPGroup, 0) #3332 - Align all_*_groups methods some more #3433 - Add all_perfect_groups #3434 - Add all_primitive_groups and all_transitive_groups variants taking a single int or int range #3404 - fix a docstring #3436 - Fixes multivariate division #3396 - Docu invariants tori #3428 - Improve docstrings for is_conjugate/is_conjugate_with_data. #3384 - Fix ambient_module(M::SubquoModule) #3448 - Bugfix for printing of affine schemes #3437 - Bugfix for bugfix for printing of affine schemes #3445 - Update OSCAR banner #3410 - Docu invariants lin. red. groups (Lakshmi Ramesh and Wolfram Decker) #3443 - add od_from_atlas_group, od_from_p_subgroup, and helpers #3444 - Unexport normalise #3453 - support group properties for character tables #3449 - add docstrings for acting_group and action_function #3432 (exports are used in new groups code for the book) - Adjust to renaming of rank(A::FinGenAbGroup) to torsion_free_rank(A::FinGenAbGroup) #3457 - Ensure fp_group(G) transfers group attributes #3464 - Added comment on convention #3467 - Export weierstrass_chart_on_minimal_model and patch transform_to_weierstrass #3458 - Fix a doc signature #3466 - Grading + caching for affine algebra of torus invariants #3469

HereAround · 2024-03-11T10:21:34Z

As it turns out, the toric varieties code uses rank on abelian groups extensively, e.g. rank(picard_group(x)) and rank(torusinvariant_cartier_divisor_group) and more.

@HereAround are these groups generally torsion free (so the two kinds of rank coincide), or not?

In general they are not torsion free, but for my applications they always are. In any case, torsion-free rank should be the correct replacement/extension.

fingolfin added the backport 1.0.x Should be backported to the release 1.0 branch label Feb 28, 2024

benlorenz mentioned this pull request Feb 28, 2024

Backports Oscar 1.0 #3429

Merged

31 tasks

fingolfin added 3 commits February 29, 2024 00:29

Prepare for renaming of rank(A::FinGenAbGroup)

b075b2b

... to torsion_free_rank(A::FinGenAbGroup), see also <thofma/Hecke.jl#1224>.

Use is_z_graded in _regularity_bound

744a80e

more rank -> torsion_free_rank

f83e591

fingolfin force-pushed the mh/torsion_free_rank branch from 3e83fda to f83e591 Compare February 29, 2024 01:05

more

2243f69

more more

6e8d544

lgoettgens reviewed Feb 29, 2024

View reviewed changes

Update experimental/GModule/Cohomology.jl

f1c0df5

joschmitt reviewed Feb 29, 2024

View reviewed changes

thofma reviewed Feb 29, 2024

View reviewed changes

more more more

4cd5d24

bump Hecke

c7ccf3b

thofma changed the title ~~Prepare for renaming of rank(A::FinGenAbGroup) to torsion_free_rank(A::FinGenAbGroup)~~ Adjust to renaming of rank(A::FinGenAbGroup) to torsion_free_rank(A::FinGenAbGroup) Feb 29, 2024

thofma closed this Feb 29, 2024

thofma reopened this Feb 29, 2024

fieker merged commit 8b863ed into oscar-system:master Feb 29, 2024
21 of 47 checks passed

benlorenz removed the backport 1.0.x Should be backported to the release 1.0 branch label Feb 29, 2024

fingolfin deleted the mh/torsion_free_rank branch March 1, 2024 08:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust to renaming of `rank(A::FinGenAbGroup)` to `torsion_free_rank(A::FinGenAbGroup)` #3457

Adjust to renaming of `rank(A::FinGenAbGroup)` to `torsion_free_rank(A::FinGenAbGroup)` #3457

fingolfin commented Feb 28, 2024

fingolfin commented Feb 28, 2024

fingolfin commented Feb 28, 2024

codecov bot commented Feb 29, 2024

benlorenz commented Feb 29, 2024

thofma commented Feb 29, 2024

benlorenz commented Feb 29, 2024

lgoettgens Feb 29, 2024

joschmitt Feb 29, 2024

lgoettgens Feb 29, 2024

lgoettgens Feb 29, 2024

joschmitt Feb 29, 2024

joschmitt Feb 29, 2024

thofma Feb 29, 2024

fieker Feb 29, 2024

thofma Feb 29, 2024

joschmitt Feb 29, 2024

fingolfin Feb 29, 2024

thofma Feb 29, 2024

thofma commented Feb 29, 2024

thofma commented Feb 29, 2024

thofma commented Feb 29, 2024

fieker commented Feb 29, 2024

HereAround commented Mar 11, 2024

	Oscar.dim(C::GModule) = torsion_free_rank(C.M)
	Oscar.dim(C::GModule) = rank(C.M)

Adjust to renaming of rank(A::FinGenAbGroup) to torsion_free_rank(A::FinGenAbGroup) #3457

Adjust to renaming of rank(A::FinGenAbGroup) to torsion_free_rank(A::FinGenAbGroup) #3457

Conversation

fingolfin commented Feb 28, 2024

fingolfin commented Feb 28, 2024

fingolfin commented Feb 28, 2024

codecov bot commented Feb 29, 2024

Codecov Report

benlorenz commented Feb 29, 2024

thofma commented Feb 29, 2024

benlorenz commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thofma commented Feb 29, 2024

thofma commented Feb 29, 2024

thofma commented Feb 29, 2024

fieker commented Feb 29, 2024

HereAround commented Mar 11, 2024

Adjust to renaming of `rank(A::FinGenAbGroup)` to `torsion_free_rank(A::FinGenAbGroup)` #3457

Adjust to renaming of `rank(A::FinGenAbGroup)` to `torsion_free_rank(A::FinGenAbGroup)` #3457