`hom` should do some argument checking and provide better error messages

[YueRen]

Sorry for the bad title. Found by @yuvraj-singh-math. If g is a polynomial, g//1 and g/1 produces two different types. This isn't problematic, as most functions do not distinguish between the resulting types, but hom does:

Code to reproduce the error:

R,(x1,x2) = polynomial_ring(QQ,["x1","x2"])
x1//2 == x1/2 # true
phi = hom(R,R,c->c,gens(R))
phi(x1//2) # does not work
phi(x1/2)  # works

(QQ above is not special, the error also occurs over a finite field or a rational function field)

Error that is produced:

julia> R,(x1,x2) = polynomial_ring(QQ,["x1","x2"])
(Multivariate polynomial ring in 2 variables over QQ, QQMPolyRingElem[x1, x2])

julia> x1//2 == x1/2
true

julia> phi = hom(R,R,c->c,gens(R))
Ring homomorphism
  from multivariate polynomial ring in 2 variables over QQ
  to multivariate polynomial ring in 2 variables over QQ
defined by
  x1 -> x1
  x2 -> x2
with map on coefficients
  #41

julia> phi(x1//2) # does not work
ERROR: MethodError: no method matching (::QQMPolyRing)(::AbstractAlgebra.Generic.FracFieldElem{QQMPolyRingElem})

Closest candidates are:
  (::QQMPolyRing)(::Vector{QQFieldElem}, ::Vector{Vector{Int64}})
   @ Nemo ~/.julia/packages/Nemo/pX5Oz/src/flint/fmpq_mpoly.jl:1236
  (::QQMPolyRing)(::Vector{Any}, ::Array{Vector{T}, 1}) where T
   @ Nemo ~/.julia/packages/Nemo/pX5Oz/src/flint/fmpq_mpoly.jl:1248
  (::QQMPolyRing)(::Vector{QQFieldElem}, ::Array{Vector{T}, 1}) where T<:Union{UInt64, ZZRingElem}
   @ Nemo ~/.julia/packages/Nemo/pX5Oz/src/flint/fmpq_mpoly.jl:1224
  ...

Stacktrace:
 [1] (::Oscar.MPolyAnyMap{…})(g::AbstractAlgebra.Generic.FracFieldElem{…})
   @ Oscar ~/.julia/dev/Oscar/src/Rings/MPolyMap/MPolyRing.jl:179
 [2] top-level scope
   @ REPL[156]:1
Some type information was truncated. Use `show(err)` to see complete types.

julia> phi(x1/2)  # works
1//2*x1

[thofma]

The object x1//2 is an element of the fraction field. That phi does not accept x1//2 but x1/2 is not a bug. For the same reason a function defined on ZZ accepts 1 but not 1//1.

Edit: The error could be better though.

[YueRen]

I see, so is this a user error and using // should be discouraged (for people working with polynomials at least)? I must admit that I got used to using // instead of / because / can produce floats which cannot be converted to QQFieldElem.

[thofma]

As usual the answer is complicated. For all the rings that Oscar introduces, x/y == divexact(x, y) (aka x * inv(y) if y is invertible in the ring). Using // will always create something in the fraction field. We cannot change what 1/1 does though (as you observed). There was a discussion in https://github.com/oscar-system/Oscar.jl/issues/1618 on the mismatch between 1/1 and ZZ(1)/ZZ(1), but we deemed it negligible compared to the advantages.

[YueRen]

Got it, I will keep that in mind going forward. Thanks for the clarification!

[thofma]

Let me repurpose this issue as a feature request to some better argument checking in the application of a morphisms.

The feedback of the people coming in touch with Oscar for the first time are quite valuable. Happy to hear suggestions on how to make things easier/clearer with respect to the issues encountered here.

[YueRen]

Is it possible to write some generic fall-back code that allows hom to be applied to the fraction field (as long as the denominator is a unit)?