Skip to content

Use symbols instead of strings for variables in toric geometry#4928

Merged
HereAround merged 9 commits intomasterfrom
lk/toric_geometry_symbols
Sep 8, 2025
Merged

Use symbols instead of strings for variables in toric geometry#4928
HereAround merged 9 commits intomasterfrom
lk/toric_geometry_symbols

Conversation

@lkastner
Copy link
Copy Markdown
Member

@lkastner lkastner commented May 28, 2025

Fixes #4159

Copy link
Copy Markdown
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thank you!

Left some optional comments, feel free to ignore.

But regarding the CI errors, here is one:

LoadError: MethodError: no method matching polynomial_ring(::QQField, ::Vector{Any}; cached::Bool)

Closest candidates are:
  polynomial_ring(::Ring, ::Vector{Symbol}; kw...)
   @ AbstractAlgebra ~/oscar-runners/runner-28/julia/packages/AbstractAlgebra/5aJmh/src/MPoly.jl:1506
  polynomial_ring(::Ring, ::Union{AbstractArray{<:Union{Char, AbstractString, Symbol}}, Pair{<:Union{Char, AbstractString, Symbol}}}, Union{AbstractArray{<:Union{Char, AbstractString, Symbol}}, Pair{<:Union{Char, AbstractString, Symbol}}}...; kv...)
   @ AbstractAlgebra ~/oscar-runners/runner-28/julia/packages/AbstractAlgebra/5aJmh/src/misc/VarNames.jl:300
  polynomial_ring(::Ring, ::Tuple{Vararg{Union{AbstractArray{<:Union{Char, AbstractString, Symbol}}, Pair{<:Union{Char, AbstractString, Symbol}}}}}; kv...)
   @ AbstractAlgebra ~/oscar-runners/runner-28/julia/packages/AbstractAlgebra/5aJmh/src/misc/VarNames.jl:302
  ...

Stacktrace:
  [1] _construct_generic_sample
    @ ~/oscar-runners/runner-28/_work/Oscar.jl/Oscar.jl/experimental/FTheoryTools/src/auxiliary.jl:393

The code triggering this:

function _construct_generic_sample(base_grading::Matrix{Int64}, base_vars::Vector{String}, d::Int, fiber_ambient_space::NormalToricVariety, fiber_twist_divisor_classes::ZZMatrix)
  base_space = family_of_spaces(polynomial_ring(QQ, base_vars, cached = false)[1], base_grading, d)
  ambient_space_vars = vcat(base_vars, coordinate_names(fiber_ambient_space))
  coordinate_ring_ambient_space = polynomial_ring(QQ, ambient_space_vars, cached = false)[1]
...

I suspect that base_vars and coordinate_names(fiber_ambient_space) now have different types (one a Vector{String} one a Vector{Symbol}).

Two solutions come to mind (and perhaps both should be implemented):
1 also drop the vcat in that method and instead write

  coordinate_ring_ambient_space = polynomial_ring(QQ, base_vars, coordinate_names(fiber_ambient_space); cached = false)[1]
  1. ensure that the Vector{String} there is also changed into a Vector{Symbol}

Comment thread experimental/Schemes/src/ToricBlowups/constructors.jl Outdated
Comment thread src/AlgebraicGeometry/ToricVarieties/NormalToricVarieties/attributes.jl Outdated
@HereAround
Copy link
Copy Markdown
Member

HereAround commented Sep 2, 2025

@lkastner and I discussed this PR earlier today. We agreed, that I would take this over to suffer through the necessary changes in FTheoryTools. Thus, I just committed some suggestions from code review and rebased to #5263.

@HereAround HereAround added release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes optimization Simpler/more performant code or more/better tests extra-long Also run the extra-long tests during CI. labels Sep 2, 2025
@HereAround HereAround mentioned this pull request Sep 2, 2025
16 tasks
@HereAround HereAround force-pushed the lk/toric_geometry_symbols branch 8 times, most recently from 67cc04e to 5517b76 Compare September 3, 2025 17:10
Comment thread experimental/Schemes/src/ToricBlowups/constructors.jl Outdated
Comment thread experimental/Schemes/src/ToricBlowups/constructors.jl Outdated
Comment thread experimental/Schemes/src/ToricBlowups/constructors.jl Outdated
Comment thread src/AlgebraicGeometry/ToricVarieties/cohomCalg/auxiliary.jl Outdated
Comment thread src/AlgebraicGeometry/ToricVarieties/NormalToricVarieties/attributes.jl Outdated
Comment thread src/AlgebraicGeometry/ToricVarieties/NormalToricVarieties/attributes.jl Outdated
Comment thread src/AlgebraicGeometry/ToricVarieties/NormalToricVarieties/attributes.jl Outdated
Comment thread experimental/Schemes/src/ToricBlowups/constructors.jl Outdated
@HereAround HereAround force-pushed the lk/toric_geometry_symbols branch 3 times, most recently from bb7ce0b to 3c45de5 Compare September 4, 2025 19:35
@HereAround HereAround marked this pull request as ready for review September 5, 2025 05:45
Copy link
Copy Markdown
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

The changes look mostly fine to me, thanks for working on this @HereAround! In a few places, I think the changes to docstrings make it harder for users to use specific kwargs (see comments).

I can not judge if this really resolves #4159 as I am not familiar enough with the surrounding code that was not touched here.

Comment thread experimental/Schemes/src/ToricBlowups/constructors.jl Outdated
Comment thread experimental/Schemes/src/ToricBlowups/constructors.jl Outdated
Comment thread experimental/Schemes/src/ToricBlowups/constructors.jl Outdated
Comment thread src/AlgebraicGeometry/ToricVarieties/NormalToricVarieties/attributes.jl Outdated
HereAround and others added 3 commits September 8, 2025 13:56
Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>
…ibutes.jl

Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>
@HereAround HereAround force-pushed the lk/toric_geometry_symbols branch from c748c7d to 4cf54da Compare September 8, 2025 12:02
@HereAround HereAround force-pushed the lk/toric_geometry_symbols branch from 4cf54da to bf0f516 Compare September 8, 2025 12:03
@HereAround
Copy link
Copy Markdown
Member

@lgoettgens seems happy. I checked with @lkastner on slack, who also seems happy. I shall thus assume that this PR is good to go and will now merge it.

@HereAround HereAround merged commit 9e3515c into master Sep 8, 2025
33 checks passed
@HereAround HereAround deleted the lk/toric_geometry_symbols branch September 8, 2025 14:02
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 topic: toric geometry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Symbols vs. strings in toric varieties

4 participants