`proj_space` vs `projective_space`

[lkastner]

Currently both of these appear and I do not know why. Yet, for affine_space there is no aff_space. It is definitely bad style to have both and I think that projective_space is the one that makes more sense (otherwise we would have to change a lot of stuff)

[lars@nomurai src]$ cat exports.jl | grep projective
export complex_projective_plane
export is_projective
export is_projective_space
export morphism_of_projective_schemes
export projective_general_linear_group
export projective_geometry
export projective_omega_group
export projective_orthogonal_group
export projective_plane
export projective_scheme
export projective_space
export projective_special_linear_group
export projective_special_orthogonal_group
export projective_special_unitary_group
export projective_symplectic_group
export projective_unitary_group
export real_projective_plane
export weighted_projective_space

Through git blame I arrived at b02ea952243 where @HechtiDerLachs changed most of the projective_space to proj_space. The styleguide even uses projective_space as an example. Also see https://github.com/oscar-system/Oscar.jl/issues/922.

Hence I would ask someone familiar with the AlgebraicGeometry section to get rid of proj_space and introduce the suitable variant of projective_space.

[lkastner]

Also there are some export statements left in these files? @HereAround why did you leave them there when you were working on the export stuff?

[HereAround]

The following may or may not be relevant, but I wanted to mention it:

There is the notion of "projectivization of the total space of a vector bundle". If everything works correct, then this uses the constructor proj in the toric setting, and so may be borderline relevant.

As for export statements (in which files btw?), there is a good chance that I simply missed them. I.e., this is a mistake and should be corrected.

I support your statement that there should be a unified command across all of the algebraic geometry, and given how close for instance the above mentioned proj (for projectivization...) is, I also do support projective_space as the unified wording/command for this.

[lkastner]

As for export statements (in which files btw?), there is a good chance that I simply missed them. I.e., this is a mistake and should be corrected.

The file is https://github.com/oscar-system/Oscar.jl/blob/73497a75ae2a1bda3a0a52d32c6f906c2097d4b6/src/AlgebraicGeometry/Miscellaneous/basics.jl#L13

It is in a module (Geometry) which is why you left it in.

[lkastner]

I think @HechtiDerLachs just did not know about the issue about how to deal with the name conflicts and its conclusion and the stuff in the style guide was only added later.

[HereAround]

As for export statements (in which files btw?), there is a good chance that I simply missed them. I.e., this is a mistake and should be corrected.

The file is

https://github.com/oscar-system/Oscar.jl/blob/73497a75ae2a1bda3a0a52d32c6f906c2097d4b6/src/AlgebraicGeometry/Miscellaneous/basics.jl#L13

It is in a module (Geometry) which is why you left it in.

Yes, those exports are in a module and cannot be removed. (There a couple of similar places within Rings if I recall correctly). For all it matters: This Miscellaneous file is a "relic" - originally created by @fieker - those functions are probably not (or no longer) used, and the last time @fieker and I discussed, I had the impression that this file could be removed. In fact, I may have opened an issue about this at some point, but my recollection is a bit vague...

[lkastner]

The proj_space seems to be used. This was the thing that made https://github.com/oscar-system/Oscar.jl/pull/2924 fail just now, which is how I stumbled onto this issue.

https://github.com/oscar-system/Oscar.jl/actions/runs/6534926172/job/17743218810?pr=2924

  Got exception outside of a @test
  MethodError: no method matching graded_polynomial_ring(::QQField, ::Pair{Symbol, UnitRange{Int64}})

  Closest candidates are:
    graded_polynomial_ring(::Ring, ::Union{Tuple{Vararg{T}}, AbstractVector{T}}) where T<:Union{Char, AbstractString, Symbol}
     @ Oscar ~/work/Oscar.jl/Oscar.jl/src/Rings/mpoly-graded.jl:412
    graded_polynomial_ring(::Ring, ::Union{Tuple{Vararg{T}}, AbstractVector{T}}, ::Any; ordering, cached) where T<:Union{Char, AbstractString, Symbol}
     @ Oscar ~/work/Oscar.jl/Oscar.jl/src/Rings/mpoly-graded.jl:412
    graded_polynomial_ring(::Ring, ::Int64, ::Union{Char, AbstractString, Symbol})
     @ Oscar ~/work/Oscar.jl/Oscar.jl/src/Rings/mpoly-graded.jl:420
    ...

  Stacktrace:
    [1] proj_space(R::QQField, n::Int64, name::Symbol)
      @ Oscar.Geometry ~/work/Oscar.jl/Oscar.jl/src/AlgebraicGeometry/Miscellaneous/basics.jl:31
    [2] proj_space(R::QQField, n::Int64)
      @ Oscar.Geometry ~/work/Oscar.jl/Oscar.jl/src/AlgebraicGeometry/Miscellaneous/basics.jl:31
    [3] macro expansion
      @ ~/work/Oscar.jl/Oscar.jl/test/Rings/solving.jl:74 [inlined]
    [4] macro expansion
      @ /opt/hostedtoolcache/julia/1.9.3/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:1498 [inlined]
    [5] top-level scope
      @ ~/work/Oscar.jl/Oscar.jl/test/Rings/solving.jl:74
    [6] include
      @ ./Base.jl:458 [inlined]
    [7] macro expansion
      @ ./timing.jl:501 [inlined]
    [8] include(str::String)
      @ Main ~/work/Oscar.jl/Oscar.jl/test/runtests.jl:91
    [9] top-level scope
      @ ~/work/Oscar.jl/Oscar.jl/test/Rings/runtests.jl:17
   [10] include
      @ ./Base.jl:458 [inlined]
   [11] macro expansion
      @ ./timing.jl:501 [inlined]
   [12] include(str::String)
      @ Main ~/work/Oscar.jl/Oscar.jl/test/runtests.jl:91
   [13] (::Base.var"#978#983"{typeof(include)})(r::Base.RefValue{Any}, args::Tuple{String})
      @ Base ./asyncmap.jl:100
   [14] macro expansion
      @ ./asyncmap.jl:234 [inlined]
   [15] (::Base.var"#994#995"{Base.var"#978#983"{typeof(include)}, Channel{Any}, Nothing})()
      @ Base ./task.jl:514

[HereAround]

Interesting. Ok - I stand corrected. We should dig deeper (like why were those "old" projective_space implementations used? Can the more involved ones in the schemes/toric setting take over?)

[lkastner]

You are right, there actually also exists projective_space with the arguments that proj_space eats. We could try out what happens if we just remove the export statement and introduce an alias.

[HereAround]

Two more thoughts:

1. As for the "why": On 2nd thought and since the error happens in testing rings, I recall that @fieker needed the projective space over rings/fields other than the complex numbers. Maybe this generalization does not work with the existing scheme code.
2. Is the issue in the above run (cannot find command/constructor/function graded_polynomial_ring) maybe related to https://github.com/oscar-system/Oscar.jl/pull/2889? If my recollection is correct, this PR introduced this notion.

[lkastner]

2. Is the issue in the above run (cannot find command/constructor/function `graded_polynomial_ring`) maybe related to [Prefer graded_polynomial_ring over grade #2889](https://github.com/oscar-system/Oscar.jl/pull/2889)? If my recollection is correct, this PR introduced this notion.

It is, graded_polynomial_ring is just missing a few signatures. I think I added those.

[lkastner]

The alias approach does not work:

julia> PP = proj_space(QQ, 2)
Projective space of dimension 2
  over rational field
with homogeneous coordinates [s0, s1, s2]

julia> P = Oscar.Geometry.ProjSpcElem(PP[1], [QQ(0), QQ(0), QQ(1)])
ERROR: MethodError: no method matching getindex(::ProjectiveVariety{QQField, MPolyDecRing{QQFieldElem, QQMPolyRing}}, ::Int64)

Closest candidates are:
  getindex(::AbsProjectiveScheme, ::AbsSpec)
   @ Oscar ~/oscars/master/src/AlgebraicGeometry/Schemes/ProjectiveSchemes/Objects/Methods.jl:348

Stacktrace:
 [1] top-level scope
   @ REPL[9]:1

This is due to

julia> typeof(projective_space(QQ,2))
ProjectiveVariety{QQField, MPolyDecRing{QQFieldElem, QQMPolyRing}}

julia> typeof(Oscar.Geometry.proj_space(QQ,2))
Tuple{Oscar.Geometry.ProjSpc{QQFieldElem}, Vector{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}}}

So this will need to be resolved.

[fingolfin]

One crucial thing about proj_space is that it provides concrete points of the projective spaces, which we need for a bunch of applications...

I don't know whether this available for the "new" code in projective_space. Here in our meeting in Kaiserslautern nobody else seems to know either. Maybe @simonbrandhorst does?

[fingolfin]

The "missing" signatures in graded_polynomial_ring are something that will be fixed this (or next) week by a completely different patch, I would not worry about them (and that they were used in my PR was an accident which I will fix there)

[simonbrandhorst]

The new code in projective_space is due to me. As far as I remember it supports what the old code does too. But it seems that I never came around removing the old code. What is still missing is something like an iterator over all the points for fields which implement an iterator themselves. Since I did not need it so far, it is not implemented.

julia> P1 = projective_space(GF(7),1)
Projective space of dimension 1
  over finite field of characteristic 7
with homogeneous coordinates [s0, s1]

julia> pt = P1([0,1])
Projective rational point
  of Projective 1-space over GF(7) with coordinates [s0, s1]
with coordinates (0 : 1)

julia> P = parent(pt)
Set of rational points
  over Spec of finite field of characteristic 7
  of projective 1-space over GF(7) with coordinates [s0, s1]

If there are further types of projective spaces we could disambiguate by making the type a first argument, similarly as for abelian_group:

projective_space(Scheme, QQ, 1)
projective_space(RationalPointSet, QQ, 1)  ?
projective_space(NormalToricVariety, 1) ? 
projective_space(MyFavouriteProjectiveSpaceType, ...) 

[HereAround]

I am not sure if I understand what you mean by "disambiguate".

The toric varieties do not (yet) follow the above naming scheme, because we always work over QQ. Of course, one can introduce a constructor with a field/ring and raise an error whenever the type NormalToricVariety is paired with anything other than QQ. I would be ok with that, as long as we keep supporting the version without QQ (I do not want to write QQ each an every time I construct a toric variety.)

I like the idea of a custom type MyFavouriteProjectiveSpaceType that a user could set to the instance/type that will be most relevant to that person (in my case likely NormalToricVariety). Maybe this could be used as fallback, i.e. a call of projective_space(QQ, 1) would call projective_space(MyFavoriteProjectiveSpaceType, QQ, 1). Maybe one could even support projective_space(1), if a default ring/field would be fixed (as is - at least currently - the case for the toric varieties.)

[simonbrandhorst]

@HereAround I edited my answer to clarify.

[HereAround]

@HereAround I edited my answer to clarify.

Thank you. That indeed clarifies the matter.

To reiterate, I like the idea of MyFavoriteProjectiveSpaceType. However, we might need to think this bigger. For instance, there are weighted projective spaces, Hirzebruch and del Pezzo surfaces, and a lot other "standard" constructions. So maybe this should be MyFavoriteSpaceType.

(Alternatively, one could try to agree on a standard/default type that is used whenever no type is specified. But I like the custom type better. It will be hard to agree on a default, and it will not be too convenient for those people that are not using the default setting and thus have to type the type...)

[fingolfin]

Just to say, we need projective_space also for $\mathbb{Z}/n\mathbb{Z}$ with $n$ not a prime. I am told this also works with the code by Simon, so that's great.

The question remains, what to do with the PlaneCurve code: we could adapt it to use the "new" projective_space, but it is worth it at this point? My understanding is that this code needs a major rewrite anyway or needs to be redone completely? It should also be tied in with other stuff (e.g. the elliptic curve work done by @JHanselman, or the work on schemes/varieties/... )

Do we have a plan and who has it and who will implement it?

[lkastner]

The question remains, what to do with the PlaneCurve code: we could adapt it to use the "new" projective_space, but it is worth it at this point?

I only want proj_space gone. If the PlaneCurve code needs a rewrite, then just disable it for now, so nobody uses it by accident.

Do we have a plan and who has it and who will implement it?

I'll gladly do the disabling and the deletion of proj_space. Just say the word.

[wdecker]

Disabling the PlaneCurve code would kill this: Rational Parametrizations of Rational Plane Curves.

[simonbrandhorst]

I once promised I would look at that code. But it is kind of low on my list of priorities since no one seems to really need it.

[lkastner]

Ok, I'll just stop caring.

[simonbrandhorst]

2988 removes the dependency on delphines code.

Once that is merged we can move the proj_space code to the code of delphine and in this way get it out of global namespace.

[fingolfin]

But proj_space is still being used in rational_solutions:

src/Rings/solving.jl:234:  P = proj_space(Q, ngens(RS))[1]
test/Rings/solving.jl:74:  Q, x = proj_space(QQ, 2)

So we first should get rid of it there, too. I think it is only used there to encode the return values, which are projective points. Maybe this can be switched to the new code, too?

[simonbrandhorst]

Removed proj_space in #3179