Introduce upgrade! for universal polynomials

[SoongNoonien]

As discussed in #2206 I've added a new upgrade! method which is better suited for the use in the universal polynomial code than upgrade was before.

Now one can even argue that the original upgrade method could be removed. I'm fine with that or leaving it as is. In #2198 things will change anyway.

[SoongNoonien]

I see where the problem is but I'm not sure if it is a problem with the change or if evaluate was broken before. The tests fail when a constant universal polynomial is evaluated at a univariate polynomial. They fail because the same universal polynomial gets evaluated prior to that which upgrades it. The upgraded polynomial is then internally a multivariate polynomial depending on three variables. And evaluate fails because it tries to map the univariate polynomial into the universal polynomial ring. Interestingly, evaluating the underlying multivariate polynomial at the same univariate polynomial fails too. So, I'm not sure if this evaluate which is tested here should work at all.

[lgoettgens]

Some opinions on this:

• The part of the tests that relies on f being never upgraded IMO assumes too much about internals.
• However, evaluating at univariate polynomials should still work, as e.g. in

  AbstractAlgebra.jl/test/generic/UnivPoly-test.jl

  Line 983 in 6a52300

  @test evaluate(h, V) == evaluate(h, [U(v) for v in V])

It would be great if @fingolfin could also have a look at this.

[SoongNoonien]

However, evaluating at univariate polynomials should still work, as e.g. in...

Then this should work for multivariate polynomials too. I mean evaluating multivariate polynomials at univariate polynomials. The line you referenced fails if we take h in a multivariate polynomial ring if V has less than three elements which can happen as its length is chosen randomly between one and three.

[lgoettgens]

I am just talking about the evaluate cases, where we substitute all variables. If we just substitute a proper subset of variables, then it should remain in the Univ poly ring (i.e. univariate polys are not allowed).

[SoongNoonien]

If we just substitute a proper subset of variables, then it should remain in the Univ poly ring

Ok, so in contrast to multivariate polynomials you think we should allow evaluating universal polynomials at the first $k$ variables, even if the ring already has more than $k$ variables?

[SoongNoonien]

I'd say we should also allow evaluating multivariate polynomials in this way. So, things like this should actually work:

julia> R,(x,y)=ZZ[:x,:y]
(Multivariate polynomial ring in 2 variables over integers, AbstractAlgebra.Generic.MPoly{BigInt}[x, y])

julia> evaluate(x+y, [1])
ERROR: ArgumentError: Incorrect number of values in evaluation
Stacktrace:
 [1] macro expansion
   @ ~/.julia/packages/AbstractAlgebra/L8iQ0/src/Assertions.jl:602 [inlined]
 [2] evaluate(a::AbstractAlgebra.Generic.MPoly{BigInt}, vals::Vector{Int64})
   @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/L8iQ0/src/MPoly.jl:874
 [3] top-level scope
   @ REPL[3]:1

I think this should simply return x+1.

[SoongNoonien]

So, I've removed the test which failed. This test depended on the creation time of the constant universal polynomial f. I'll look into the evaluation of universal polynomials to make it independent of the creation time of the polynomials. I've talked with @fingolfin and @fieker about how this could be done.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.97%. Comparing base (f97729d) to head (3b57ed6).
⚠️ Report is 18 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2211      +/-   ##
==========================================
- Coverage   88.01%   87.97%   -0.04%     
==========================================
  Files         127      127              
  Lines       31765    31749      -16     
==========================================
- Hits        27957    27932      -25     
- Misses       3808     3817       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[thofma]

I'd say we should also allow evaluating multivariate polynomials in this way. So, things like this should actually work:

julia> R,(x,y)=ZZ[:x,:y]
(Multivariate polynomial ring in 2 variables over integers, AbstractAlgebra.Generic.MPoly{BigInt}[x, y])

julia> evaluate(x+y, [1])
ERROR: ArgumentError: Incorrect number of values in evaluation
Stacktrace:
 [1] macro expansion
   @ ~/.julia/packages/AbstractAlgebra/L8iQ0/src/Assertions.jl:602 [inlined]
 [2] evaluate(a::AbstractAlgebra.Generic.MPoly{BigInt}, vals::Vector{Int64})
   @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/L8iQ0/src/MPoly.jl:874
 [3] top-level scope
   @ REPL[3]:1

I think this should simply return x+1.

I don't like to always say "no", but I would prefer for this to still error.

Edit: There is no need to be "consistent" with universal polynomials. I think they don't have a well-defined notion of "number of variables" anyway.

[SoongNoonien]

I don't like to always say "no", but I would prefer for this to still error.

With #2214 this still errors, even for universal polynomials.

[fingolfin]

I think this PR is good as it is and should be merged so that we can progress with other work. The situation with evaluate(f, ...) vs subst(f, ...) vs f(...) is something we should also tackle, but IMHO it is not very relevant for this PR. For it just makes an existing issue with evaluate for universal polynomial rings more prominent -- after all, one can make a slightly different version of the removed test which will trigger the issue also in the current version of the code.

I'll wait a bit with merging in case @thofma or @lgoettgens (or anyone else for that matter) has some final concerns -- but my impression from the discussion so far was that nobody really objected to this PR, the discussion was more focused on how to "fix" evaluate (here by "fix" I include the option "make sure it keeps erroring, but more consistently")