Missing signature or argument checks for `hom` between `MPolyAnyRing`

Describe the bug While constructing a homomorphism between two multivariate polynomial rings, the function do not check the entry which leads to a bug when trying to print.

I was used to create group homomorphism by feeding a domain, a codomain, a set of generators and their images. I thought that it was similar for hom between rings but apparently not and the constructor did not complain (I then checked the source code and found out my mistake. The problem is that there are many functions hom and they are not consistent through Oscar.

To Reproduce That was my particular example, where I just wanted to map a polynomial ring to its reduction modulo a prime ideal.

using Oscar

E, w = cyclotomic_field(21);
ww3 = w^7;
ww7 = w^3;
OE = maximal_order(E);
dec = prime_decomposition(OE,127);
P = dec[1][1];
R, h = Hecke.ResidueFieldSmall(OE,P);
hext=extend(h,E);
K, (x11,x12,x13,x21,x22,x23,x31,x32,x33) = polynomial_ring(E, ["x11", "x12", "x13", "x21", "x22", "x23", "x31", "x32", "x33"]);

Kp = parent(map_coefficients(hext, x11))

KtoKp = hom(K, Kp, gens(K), gens(Kp))

Expected behavior I did not expect much but while trying to print KtoKp, I end up with an error which is not super explicit. Indeed, it does not tell me I did something wrong with the constructor, but just fallback on trying to use a vector as a function:

julia> KtoKp = hom(K, Kp, gens(K), gens(Kp))
Ring homomorphism
  from multivariate polynomial ring in 9 variables over cyclotomic field of order 21
  to multivariate polynomial ring in 9 variables over GF(127)
defined by
Error showing value of type Oscar.MPolyAnyMap{AbstractAlgebra.Generic.MPolyRing{nf_elem}, fqPolyRepMPolyRing, Vector{AbstractAlgebra.Generic.MPoly{nf_elem}}, fqPolyRepMPolyRingElem}:
ERROR: MethodError: objects of type Vector{AbstractAlgebra.Generic.MPoly{nf_elem}} are not callable
Use square brackets [] for indexing an Array.
Stacktrace:
  [1] map_coefficients(f::Vector{AbstractAlgebra.Generic.MPoly{nf_elem}}, p::AbstractAlgebra.Generic.MPoly{nf_elem})
    @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/dsta0/src/MPoly.jl:1266
  [2] _evaluate_general(F::Oscar.MPolyAnyMap{AbstractAlgebra.Generic.MPolyRing{nf_elem}, fqPolyRepMPolyRing, Vector{AbstractAlgebra.Generic.MPoly{nf_elem}}, fqPolyRepMPolyRingElem}, u::AbstractAlgebra.Generic.MPoly{nf_elem})
    @ Oscar ~/.julia/dev/Oscar/src/Rings/MPolyMap/MPolyRing.jl:149
  [3] (::Oscar.MPolyAnyMap{AbstractAlgebra.Generic.MPolyRing{nf_elem}, fqPolyRepMPolyRing, Vector{AbstractAlgebra.Generic.MPoly{nf_elem}}, fqPolyRepMPolyRingElem})(g::AbstractAlgebra.Generic.MPoly{nf_elem})
    @ Oscar ~/.julia/dev/Oscar/src/Rings/MPolyMap/MPolyRing.jl:169
  [4] show(io::IOContext{Base.TTY}, #unused#::MIME{Symbol("text/plain")}, f::Oscar.MPolyAnyMap{AbstractAlgebra.Generic.MPolyRing{nf_elem}, fqPolyRepMPolyRing, Vector{AbstractAlgebra.Generic.MPoly{nf_elem}}, fqPolyRepMPolyRingElem})
    @ Oscar ~/.julia/dev/Oscar/src/Rings/MPolyMap/MPolyAnyMap.jl:93
  [5] (::REPL.var"#55#56"{REPL.REPLDisplay{REPL.LineEditREPL}, MIME{Symbol("text/plain")}, Base.RefValue{Any}})(io::Any)
    @ REPL /opt/julia-1.9.3/share/julia/stdlib/v1.9/REPL/src/REPL.jl:276
  [6] with_repl_linfo(f::Any, repl::REPL.LineEditREPL)
    @ REPL /opt/julia-1.9.3/share/julia/stdlib/v1.9/REPL/src/REPL.jl:557
  [7] display(d::REPL.REPLDisplay, mime::MIME{Symbol("text/plain")}, x::Any)
    @ REPL /opt/julia-1.9.3/share/julia/stdlib/v1.9/REPL/src/REPL.jl:262
  [8] display
    @ /opt/julia-1.9.3/share/julia/stdlib/v1.9/REPL/src/REPL.jl:281 [inlined]
  [9] display(x::Any)
    @ Base.Multimedia ./multimedia.jl:340
 [10] #invokelatest#2
    @ ./essentials.jl:819 [inlined]
 [11] invokelatest
    @ ./essentials.jl:816 [inlined]
 [12] print_response(errio::IO, response::Any, show_value::Bool, have_color::Bool, specialdisplay::Union{Nothing, AbstractDisplay})
    @ REPL /opt/julia-1.9.3/share/julia/stdlib/v1.9/REPL/src/REPL.jl:305
 [13] (::REPL.var"#57#58"{REPL.LineEditREPL, Pair{Any, Bool}, Bool, Bool})(io::Any)
    @ REPL /opt/julia-1.9.3/share/julia/stdlib/v1.9/REPL/src/REPL.jl:287
 [14] with_repl_linfo(f::Any, repl::REPL.LineEditREPL)
    @ REPL /opt/julia-1.9.3/share/julia/stdlib/v1.9/REPL/src/REPL.jl:557
 [15] print_response(repl::REPL.AbstractREPL, response::Any, show_value::Bool, have_color::Bool)
    @ REPL /opt/julia-1.9.3/share/julia/stdlib/v1.9/REPL/src/REPL.jl:285
 [16] (::REPL.var"#do_respond#80"{Bool, Bool, REPL.var"#93#103"{REPL.LineEditREPL, REPL.REPLHistoryProvider}, REPL.LineEditREPL, REPL.LineEdit.Prompt})(s::REPL.LineEdit.MIState, buf::Any, ok::Bool)
    @ REPL /opt/julia-1.9.3/share/julia/stdlib/v1.9/REPL/src/REPL.jl:899
 [17] (::REPL.var"#98#108"{Regex, Regex, Int64, Int64, REPL.LineEdit.Prompt, REPL.LineEdit.Prompt, REPL.LineEdit.Prompt})(::REPL.LineEdit.MIState, ::Any, ::Vararg{Any})
    @ REPL /opt/julia-1.9.3/share/julia/stdlib/v1.9/REPL/src/REPL.jl:1236
 [18] #invokelatest#2
    @ ./essentials.jl:819 [inlined]
 [19] invokelatest
    @ ./essentials.jl:816 [inlined]
 [20] (::REPL.LineEdit.var"#27#28"{REPL.var"#98#108"{Regex, Regex, Int64, Int64, REPL.LineEdit.Prompt, REPL.LineEdit.Prompt, REPL.LineEdit.Prompt}, String})(s::Any, p::Any)
    @ REPL.LineEdit /opt/julia-1.9.3/share/julia/stdlib/v1.9/REPL/src/LineEdit.jl:1603
 [21] prompt!(term::REPL.Terminals.TextTerminal, prompt::REPL.LineEdit.ModalInterface, s::REPL.LineEdit.MIState)
    @ REPL.LineEdit /opt/julia-1.9.3/share/julia/stdlib/v1.9/REPL/src/LineEdit.jl:2740
 [22] run_interface(terminal::REPL.Terminals.TextTerminal, m::REPL.LineEdit.ModalInterface, s::REPL.LineEdit.MIState)
    @ REPL.LineEdit /opt/julia-1.9.3/share/julia/stdlib/v1.9/REPL/src/LineEdit.jl:2642
 [23] run_frontend(repl::REPL.LineEditREPL, backend::REPL.REPLBackendRef)
    @ REPL /opt/julia-1.9.3/share/julia/stdlib/v1.9/REPL/src/REPL.jl:1300
 [24] (::REPL.var"#62#68"{REPL.LineEditREPL, REPL.REPLBackendRef})()
    @ REPL ./task.jl:514

So my expectation would have been that Oscar complained that my input were not correct: it should check that the user follows the documentation and put a function on the base rings as third argument. At least now I know, but young users this is not helpful if you use hom with different structures within Oscar.

System (please complete the following information): Please paste the output of Oscar.versioninfo(full=true) below. If this does not work, please paste the output of Julia's versioninfo() and your Oscar version.

julia> Oscar.versioninfo(full=true)
OSCAR version 0.14.0-DEV - #main, a5288c12f5 -- 2023-12-05 18:30:26 +0100
  combining:
    AbstractAlgebra.jl   v0.33.0
    GAP.jl               v0.10.0
    Hecke.jl             v0.22.9
    Nemo.jl              v0.37.5
    Polymake.jl          v0.11.9
    Singular.jl          v0.20.0
  building on:
    Antic_jll               v0.201.500+0
    Arb_jll                 v200.2300.0+0
    Calcium_jll             v0.401.100+0
    FLINT_jll               v200.900.7+0
    GAP_jll                 v400.1200.200+7
    Singular_jll            v403.210.1000+0
    libpolymake_julia_jll   v0.11.2+0
    libsingular_julia_jll   v0.40.7+0
    polymake_jll            v400.1100.1+0
See `]st -m` for a full list of dependencies.

Julia Version 1.9.3
Commit bed2cd540a1 (2023-08-24 14:43 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 12 × Intel(R) Xeon(R) Gold 6128 CPU @ 3.40GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, skylake-avx512)
  Threads: 1 on 12 virtual cores
Environment:
  JULIA_IMAGE_THREADS = 1
Official https://julialang.org/ release
...

Additional context I know how to solve this issue for myself, I thought though it would be good to tell about this issue (hopefully there were no other open issues on the same subject)

I guess the problem in this case is that the 'optional' third argument of hom for polynomial rings is a map for the coefficient rings and there is no real meaningful type constraint for this (it could be an OSCAR homomorphism type, a julia function, possibly a dictionary (?), ???). But I agree that the different hom constructors should be consistent. Maybe things like coeff_map should be a keyword argument? Looking at ?hom, it seems difficult though to find a common set of input arguments every hom should support. Even codomain and domain as first arguments seems not to be necessary/consistent:

  hom(A::Vector{GrpAbFinGenElem}, B::Vector{GrpAbFinGenElem}) -> Map

That there is no type restriction on the coefficient map as the third argument was a decision (If I recall correctly; @thofma ?). Whether we want to have it as a keyword argument might be debatable. I have no strong opinion on that. But specifying two sets of generators in a constructor also seems awkward to me. Why would you specify generators in the domain?

I do not think about a general solution, and at the stage where we are it is going to be difficult to revert everything to a clean and universal pattern. Though I am concerned that the constructor allows me to create something which does not work. After further investigation, it is not only the printing which is affected but also the use of the function itself.

I specify generators because there are constructors for group homomorphisms where it is possible, namely when you know the images of certain elements which do not correspond to gens for instance. In that case, you specify which are those elements and what are their image. Naively I thought it would work the same, and the constructor did not complain.

I think that there are different things to consider here:

I agree with Stevell, that in the context of groups it is a very useful feature. Assume $G, H$ are two groups and $f \colon G \to H$ a morphism that one would like to construct. Often one knows $f$ on some generating set of $G$, which might be different from the generating set of $G$. Not sure how useful it is for polynomial rings/affine algebras. Is there a situation where you know $f \colon \mathbf{Q}[x, y] \to R$ for only $f(x + 1)$ and $f(y - 1)$? There aren't that many other algebra generators.
We cannot properly type the coefficient map, because there is not proper type for callable things.

We are at a stage, where we can change everything, so we should strive for some consistency. Things we could do:

Instead hom(R, S, coefficient_map, imgs_of_gens) we do as @HechtiDerLachs suggested:

hom(R, S, imgs_of_gens [, coefficient_map = g]) # what we have now for affine algebras, but with keyword argument

Turn hom(A::Vector{GrpAbFinGenElem}, B::Vector{GrpAbFinGenElem}) into
```
hom(D, C, [a => b for (a, b) in zip(A, B)])
```
so that this is invoked for example as hom(D, C, [g1 => h1, g2 => h2]). (I also added domain/codomain here.)

I am in favour of 1), no matter what we decide else.

On Thu, Dec 07, 2023 at 12:55:53AM -0800, Tommy Hofmann wrote:

I think that there are different things to consider here:

I agree with Stevell, that in the context of groups it is a very useful feature. Assume $G, H$ are two groups and $f \colon G \to H$ a morphism that one would like to construct. Often one knows $f$ on some generating set of $G$, which might be different from the generating set of $G$. Not sure how useful it is for polynomial rings/affine algebras. Is there a situation where you know $f \colon \mathbf{Q}[x, y] \to R$ for only $f(x + 1)$ and $f(y - 1)$? There aren't that many other algebra generators.

We cannot properly type the coefficient map, because there is not proper type for callable things.

We are at a stage, where we can change everything, so we should strive for some consistency. Things we could do:
Instead hom(R, S, coefficient_map, imgs_of_gens) we do as @HechtiDerLachs suggested:
hom(R, S, imgs_of_gens [, coefficient_map = g]) # what we have now for affine algebras, but with keyword argument
I can live with both, the 1st is more concise, the 2nd readable. Is the coeff map from coeff(R) -> S or to coeff(S)? or undecided? The latter is much more efficient, but restrictive
Turn hom(A::Vector{GrpAbFinGenElem}, B::Vector{GrpAbFinGenElem}) into
hom(D, C, [a => b for (a, b) in zip(A, B)])
so that this is invoked for example as hom(D, C, [g1 => h1, g2 => h2]). (I also added domain/codomain here.) Happy with pairs. Code should probably be rewritten to directly get pairs and not the individual lists first I am in favour of 1), no matter what we decide else.
-- Reply to this email directly or view it on GitHub: https://github.com/oscar-system/Oscar.jl/issues/3072#issuecomment-1844935952 You are receiving this because you are subscribed to this thread.

Message ID: @.***>

Looking at ?hom I have the impression that we could enforce the following general policy to make hom more consistent without too much trouble. Every hom that returns a map between two objects (apparently there are others) should have one of the following signatures:

hom(domain, codomain, images, ...) defining the map by images of canonical generators
hom(domain, codomain, mapping, ...) defining the map by a julia function or a matrix or ???
hom(domain, codomain, generators, images, ...) defining the map by mapping generators to images, potentially changed to @thofma's suggestion 2

, ... can then be any other arguments that are necessary or useful to construct the map, preferably by keyword arguments.

Is the coeff map from coeff(R) -> S or to coeff(S)? or undecided?

Undecided. The code for the mappings checks which one is the case and performs the mapping accordingly.

oscar-system / Oscar.jl

Missing signature or argument checks for `hom` between `MPolyAnyRing` #3072