@varnames_interface: handle polynomial_ring(QQ, :x => 3) better

[fingolfin]

Right now polynomial_ring(QQ, :x => 3) leads to an error that is not very helpful:

julia> polynomial_ring(QQ, :x => 3)
ERROR: MethodError: no method matching iterate(::Symbol)

Closest candidates are:
  iterate(::Core.Compiler.InstructionStream, ::Int64)
   @ Base show.jl:2778
  iterate(::Core.Compiler.InstructionStream)
   @ Base show.jl:2778
  iterate(::Oscar.EdgeIterator)
   @ Oscar ~/Projekte/OSCAR/Oscar.spielwiese/src/Combinatorics/Graphs/functions.jl:352
  ...

Stacktrace:
  [1] iterate
    @ ./generator.jl:44 [inlined]
  [2] iterate
    @ ./iterators.jl:1202 [inlined]
  [3] iterate
    @ ./iterators.jl:1196 [inlined]
  [4] _collect(::Type{Symbol}, itr::Base.Iterators.Flatten{Base.Generator{…}}, isz::Base.SizeUnknown)
    @ Base ./array.jl:700
  [5] collect(::Type{Symbol}, itr::Base.Iterators.Flatten{Base.Generator{Tuple{…}, AbstractAlgebra.var"#392#393"{…}}})
    @ Base ./array.jl:694
  [6] variable_names(as::Tuple{Pair{Symbol, Int64}}, brackets::Val{true})
    @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/tABFQ/src/misc/VarNames.jl:73
  [7] variable_names(as::Pair{Symbol, Int64})
    @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/tABFQ/src/misc/VarNames.jl:71
  [8] polynomial_ring(R::QQField, s::Tuple{Pair{Symbol, Int64}}; kv::@Kwargs{})
    @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/tABFQ/src/misc/VarNames.jl:305
  [9] polynomial_ring(R::QQField, s::Tuple{Pair{Symbol, Int64}})
    @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/tABFQ/src/misc/VarNames.jl:304
 [10] polynomial_ring(::QQField, ::Pair{Symbol, Int64}; kv::@Kwargs{})
    @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/tABFQ/src/misc/VarNames.jl:302
 [11] top-level scope
    @ REPL[63]:1
Some type information was truncated. Use `show(err)` to see complete types.

I'd like to improve this. The two major options are:

1. catch the problem earlier and give a more helpful error,
2. pick an interpretation and make it work
   1. perhaps it should be the same as polynomial_ring(QQ, :x => 1:3)
   2. perhaps it should be the same as polynomial_ring(QQ, :x => 3:3)

The fact that there are at least two interpretations might suggest we should prefer the error. Then again I am not sure how natural each is? For me I comparatively often make this mistake because I think "I want three x-variables", so I use :x => 3 when I really mean :x => 1:3. The only reason I am considering the other option is that it is based on Julia semantics: one can "iterate" over an integer, and the integer is treated as if it was a 0-dimensional container with a single entry (itself). Honestly I don't like this, but it is what it is, so one might just lean into it...

Still the safe route would be 1. An error can always be turned into working code later on.

Any thoughts on this?

[lgoettgens]

The complicated case is something like polynomial_ring(QQ, :x => (1:3, 5, 1:3)). Do we wanna error here as well? Currently, this behaves as 2.ii. above (so taking 5 as 5:5). Changing this could break downstream.

The two ways I see here:
2. ii. as above (so taking 5 as 5:5)
3. only error in cases where _variable_names returns a single symbol instead of a vector. This then fails in case of polynomial_ring(QQ, :x => 5) but still allows polynomial_ring(QQ, :x => (1:3, 5, 1:3)).

[fingolfin]

I think allowing :x => (1:3, 5, 1:3) while :x => 5 fails is a misfeature if not a bug. Are you aware of any code or anyone actually relying on this? Otherwise I'd just make that illegal as we..

[lgoettgens]

I think allowing :x => (1:3, 5, 1:3) while :x => 5 fails is a misfeature if not a bug. Are you aware of any code or anyone actually relying on this? Otherwise I'd just make that illegal as we..

I thought I have seen this in a doctest somewhere in VarNames.jl, but after looking for it again, I don't find it anymore. I thus agree with you that 3 is not a good option.

[lgoettgens]

Just found

AbstractAlgebra.jl/src/misc/VarNames.jl

Line 19 in 6be27f6

Here `iter` is supposed to be any iterable, typically a range like `1:5`.

in a related docstring. This currently documents 2.ii., although it does not work.