Skip to content

Introduce class_group_with_map and picard_group_with_map#5064

Merged
fieker merged 1 commit intooscar-system:masterfrom
HereAround:ClassGroup
Aug 20, 2025
Merged

Introduce class_group_with_map and picard_group_with_map#5064
fieker merged 1 commit intooscar-system:masterfrom
HereAround:ClassGroup

Conversation

@HereAround
Copy link
Copy Markdown
Member

@HereAround HereAround commented Jul 3, 2025

This PR aims to fix #1095.

The basic idea is simple: Change the return value of the functions class_group and picard_group so that they each return a pair of the corresponding group and an interpretation map. Specifically, I envision:

  1. Return the class group and the map of the torus invariant Weil divisors into the class group.
  2. Return the Picard group and the map of the torus invariant Cartier divisors into the Picard group.

This is desired by @fieker for consistency across OSCAR.

Sadly, changing as such is a massively breaking change (see below) and cannot happen before the OSCAR 2.0 release. Therefore we proceed in two steps.

Step 1: In this PR:

  • Introduce class_group_with_map,
  • Replace all calls of class_group(...) with calls of class_group_with_map(...)
  • Introduce picard_group_with_map,
  • Replace all calls of picard_group(...) with calls of picard_group_with_map(...)
  • It is not needed to have these groups as attributes, (nor the return value of the corresponding ..._with_map function) since the underlying maps (say the map from the torus invariant Weil divisors to the class group) are attributes. Cf. Serialize mappings of properties of toric varieties and update QSMDB #5198. So I removed the corresponding attr macro from these functions.
  • By the same token, it is not needed to serialize class_group any more. So I removed this from the serialization. Note however that there are .mrdi files of toric varieties out there, which remember the class group. And they still load. A check for this are the zenodo files loaded by t = literature_model(arxiv_id = "1511.03209") and fully_resolved_big_model = resolve(t, 1). Those are loaded as part of the extra long tests, which thus serves as a check that loading such .mrdi files still works with the changes in this PR.

Step 2: With the transition to OSCAR 2.0

  1. Remove the function class_group( ) and picard_group( ) for toric varieties.
  2. Rename class_group_with_map to class_group.
  3. Rename picard_group_with_map to picard_group.
  4. Crucial: We want to take into account .mrdi files created before this PR. If such a file saved a toric variety, then it may remember the class_group. The following function takes this into account. In order to load such "old" .mrdi files consistently, the case of has_attribute(v, :class_group) in the following code snippet must not be removed:
@attr FinGenAbGroupHom function map_from_torusinvariant_weil_divisor_group_to_class_group(v::NormalToricVarietyType)
    map1 = cokernel(map_from_character_lattice_to_torusinvariant_weil_divisor_group(v))[2]
    map2 = inv(snf(codomain(map1))[2])
    # we cannot call class_group unless the attribute exists
    # but we need to make sure to have the correct codomain if it does exist
    if has_attribute(v, :class_group)
      cg = get_attribute(v, :class_group)
      return map1*map2*hom(codomain(map2), cg, gens(cg))
    else
      return map1*map2
    end
end

Of course, we must open an issue with the content of step 2 and tag it to the OSCAR 2.0 release, so that this is not forgotten.

@HereAround HereAround added topic: toric geometry optimization Simpler/more performant code or more/better tests release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Jul 3, 2025
@HereAround HereAround force-pushed the ClassGroup branch 7 times, most recently from a88c511 to 3f4c551 Compare July 4, 2025 06:50
@fingolfin fingolfin removed the triage label Jul 9, 2025
@fingolfin
Copy link
Copy Markdown
Member

Unfortunately it is breaking.

Suggestion was to maybe introduce class_group_with_map (or so). @fieker pointed out why this is nice -- and I'd agree in an ideal world, but we are stuck for now. Introducing the new name has the advantage that people can start switching to that new interface and in OSCAR 2.0 then have an easier time transitioning.

@HereAround HereAround force-pushed the ClassGroup branch 12 times, most recently from c715420 to 97e38e5 Compare August 5, 2025 14:43
@HereAround
Copy link
Copy Markdown
Member Author

@HereAround HereAround force-pushed the ClassGroup branch 4 times, most recently from 1e315b9 to c3d8222 Compare August 19, 2025 08:05
@HereAround HereAround added extra-long Also run the extra-long tests during CI. serialization labels Aug 19, 2025
@HereAround HereAround closed this Aug 19, 2025
@HereAround HereAround reopened this Aug 19, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 19, 2025

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.85%. Comparing base (b5c5ed3) to head (fb3a411).
⚠️ Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
.../ToricVarieties/NormalToricVarieties/attributes.jl 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5064      +/-   ##
==========================================
- Coverage   84.85%   84.85%   -0.01%     
==========================================
  Files         710      710              
  Lines       95593    95599       +6     
==========================================
+ Hits        81114    81118       +4     
- Misses      14479    14481       +2     
Files with missing lines Coverage Δ
.../FTheoryTools/src/LiteratureModels/constructors.jl 92.46% <100.00%> (ø)
experimental/FTheoryTools/src/auxiliary.jl 77.20% <100.00%> (ø)
.../ToricVarieties/NormalToricVarieties/properties.jl 90.32% <100.00%> (+0.32%) ⬆️
...y/ToricVarieties/ToricDivisorClasses/attributes.jl 100.00% <ø> (ø)
...ToricVarieties/ToricDivisorClasses/constructors.jl 80.76% <100.00%> (ø)
...ry/ToricVarieties/ToricLineBundles/constructors.jl 90.00% <100.00%> (ø)
...rieties/ToricLineBundles/standard_constructions.jl 100.00% <100.00%> (ø)
...cVarieties/cohomCalg/VanishingSets/constructors.jl 66.66% <100.00%> (ø)
...raicGeometry/ToricVarieties/cohomCalg/auxiliary.jl 97.14% <100.00%> (ø)
src/Serialization/ToricGeometry.jl 100.00% <ø> (ø)
... and 1 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@HereAround HereAround changed the title ToricVarieties: Return object and map Introduce class_group_with_map and picard_group_with_map Aug 19, 2025
@HereAround HereAround added release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 19, 2025
@HereAround HereAround marked this pull request as ready for review August 19, 2025 10:16
@fieker fieker merged commit 787a091 into oscar-system:master Aug 20, 2025
69 of 80 checks passed
@lgoettgens lgoettgens changed the title Introduce class_group_with_map and picard_group_with_map Introduce class_group_with_map and picard_group_with_map Aug 26, 2025
@HereAround HereAround deleted the ClassGroup branch November 27, 2025 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

extra-long Also run the extra-long tests during CI. optimization Simpler/more performant code or more/better tests release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes serialization topic: toric geometry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Toric Varieties: Should class_group return a group and a morphism?

3 participants