Change ideal_of_linear_relations to use a graded ring

[fingolfin]

This does not change the behavior of the code; but perhaps the current behavior is wrong? Unclear ... I'll leave it to @HereAround to decide what is right or wrong here :-)

[HereAround]

Thank you for attempting to fix this @fingolfin.

I am afraid we cannot do it like this, since the grading is required (at least formally to match the theory). See comment below.

[HereAround]

This needs the graded polynomial ring (with standard grading, i.e. all variable receive degree 1), so for instance:

Suggested change

	@attr MPolyIdeal function ideal_of_linear_relations(v::NormalToricVarietyType)
	R, _ = polynomial_ring(coefficient_ring(v), coordinate_names(v))
	@attr MPolyIdeal function ideal_of_linear_relations(v::NormalToricVarietyType)
	R, _ = graded_polynomial_ring(coefficient_ring(v), coordinate_names(v), cached = false)

Also needs cached = false, unless I am mistaken.

[fingolfin]

OK, so that means the current code is buggy?

[fingolfin]

(because it calls grade and then throws away the result)

[HereAround]

The grading is currently never used, or so I believe. I (tried to) add it, in order to mimic the theory, which says "the ideal of linear relations sits in a graded polynomial ring...".

[fingolfin]

OK, changed this PR now

[HereAround]

(Aside: #2942 was meant to fix this, and a few other things. But there is no problem in rebasing. So I suggest we proceed with this PR, merge it and I rebase/investigate the other PR eventually...)

[codecov]

Codecov Report

Merging #2944 (d7ca2ee) into master (866b277) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2944   +/-   ##
=======================================
  Coverage   80.29%   80.29%           
=======================================
  Files         468      468           
  Lines       66461    66459    -2     
=======================================
- Hits        53362    53361    -1     
+ Misses      13099    13098    -1     

Files	Coverage Δ
.../ToricVarieties/NormalToricVarieties/attributes.jl	98.61% <ø> (-0.02%)	⬇️

... and 1 file with indirect coverage changes

[HereAround]

Thank you @fingolfin . Looks good and can be merged once the tests pass.

[benlorenz]

The failure seems to be an honest error since the return type doesn't match for _blowup_global_sequence anymore:

  lin_i5_s = ideal_of_linear_relations(tas);
  id_fin,  = _blowup_global_sequence(id_i5_s, [[7, 8, 6], [2, 3, 1], [3, 4], [2, 4]], irr_i5_s, sri_i5_s, lin_i5_s)

Blowups of global Tate models: Error During Test at /home/runner/work/Oscar.jl/Oscar.jl/experimental/FTheoryTools/test/tate.jl:188
  Got exception outside of a @test
  MethodError: no method matching _blowup_global_sequence(::MPolyIdeal{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}}, ::Vector{Vector{Int64}}, ::MPolyIdeal{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}}, ::MPolyIdeal{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}}, ::MPolyIdeal{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}})
  
  Closest candidates are:
    _blowup_global_sequence(::T, ::Vector{<:Vector{<:Integer}}, ::T, ::T, ::MPolyIdeal{QQMPolyRingElem}; index) where T<:(MPolyIdeal{<:MPolyRingElem})
     @ Oscar ~/work/Oscar.jl/Oscar.jl/experimental/FTheoryTools/src/auxiliary.jl:350
    _blowup_global_sequence(::MPolyIdeal{QQMPolyRingElem}, ::Vector{<:Vector{<:Integer}}, ::MPolyIdeal{QQMPolyRingElem}, ::MPolyIdeal{QQMPolyRingElem}, ::MPolyIdeal{QQMPolyRingElem}; index)
     @ Oscar ~/work/Oscar.jl/Oscar.jl/experimental/FTheoryTools/src/auxiliary.jl:325
  
  Stacktrace:
    [1] macro expansion
      @ ~/work/Oscar.jl/Oscar.jl/experimental/FTheoryTools/test/tate.jl:194 [inlined]

[HereAround]

The failure seems to be an honest error since the return type doesn't match for _blowup_global_sequence anymore:

  lin_i5_s = ideal_of_linear_relations(tas);
  id_fin,  = _blowup_global_sequence(id_i5_s, [[7, 8, 6], [2, 3, 1], [3, 4], [2, 4]], irr_i5_s, sri_i5_s, lin_i5_s)

Blowups of global Tate models: Error During Test at /home/runner/work/Oscar.jl/Oscar.jl/experimental/FTheoryTools/test/tate.jl:188
  Got exception outside of a @test
  MethodError: no method matching _blowup_global_sequence(::MPolyIdeal{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}}, ::Vector{Vector{Int64}}, ::MPolyIdeal{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}}, ::MPolyIdeal{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}}, ::MPolyIdeal{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}})
  
  Closest candidates are:
    _blowup_global_sequence(::T, ::Vector{<:Vector{<:Integer}}, ::T, ::T, ::MPolyIdeal{QQMPolyRingElem}; index) where T<:(MPolyIdeal{<:MPolyRingElem})
     @ Oscar ~/work/Oscar.jl/Oscar.jl/experimental/FTheoryTools/src/auxiliary.jl:350
    _blowup_global_sequence(::MPolyIdeal{QQMPolyRingElem}, ::Vector{<:Vector{<:Integer}}, ::MPolyIdeal{QQMPolyRingElem}, ::MPolyIdeal{QQMPolyRingElem}, ::MPolyIdeal{QQMPolyRingElem}; index)
     @ Oscar ~/work/Oscar.jl/Oscar.jl/experimental/FTheoryTools/src/auxiliary.jl:325
  
  Stacktrace:
    [1] macro expansion
      @ ~/work/Oscar.jl/Oscar.jl/experimental/FTheoryTools/test/tate.jl:194 [inlined]

I see the issue and just tried to fix it. But I will probably need help from @apturner for this...

[thofma]

The ::MPolyIdeal{QQMPolyRingElem} in the signatures look suspicious. Maybe try ::MPolyIdeal{<:QQMPolyRingElem}.

[HereAround]

Thanks to @apturner , I was just able to push a fix for the honest failures. In addition, I added catches for edge cases (trying to construct a projective space of negative dimension and alike) which were originally (and still are) in #2942, but feels a bit of an overkill to have a PR that changes 3 lines...