Bandaid for polynomial evaluation

[fchapoton]

There was a problem when evaluating polynomials: the following was failing

sage: x,y=polygens(QQ,'x,y')
sage: t=PolynomialRing(x.parent(),'t').gen()
sage: F=x*y*t
sage: F(x=1)
y*t

I propose a fix that hopefully does not break anything else.
We look at a non-zero coefficient to try to guess the correct new base ring, instead of looking at the constant term.

This does not fix the following inconsistent behaviour:

sage: x.parent().zero()(x=1)
0
sage: parent(_)
Rational Field
sage: x.parent().one()(x=1)
1
sage: parent(_)
Multivariate Polynomial Ring in x, y over Rational Field

📝 Checklist

• The title is concise, informative, and self-explanatory.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation accordingly.

[mezzarobba]

Given that you call this a bandaid, it looks like we agree that the actual bug is that partial evaluations of multivariate polynomials sometimes land in the wrong parent. I don't mind working around this issue as you did, but I think it would be good to add a comment or two to explain what you are doing.

[mezzarobba]

I don't understand what you mean here.

[fchapoton]

Well, I am not 100% sure what the code is doing exactly. It seem to try to guess a new base ring, depending not only on the existing base ring, but on the substituted coefficients. But it does that by looking only at the substitution in one of the coefficients. Before my changes, it was looking at the substitution in the constant term, and fails if this was zero. I changed it to use the substitution of the leading term. This works a little better.

Of course, the only thing making complete sense would be to look at the common parent of all substituted coeffs. But this may have a large speed penalty..

[mezzarobba]

But it does that by looking only at the substitution in one of the coefficients.

Yes, but for this whole business (of evaluating the coefficients of a polynomial and getting a new polynomial) to make sense, evaluation (of the coefficients) should be a ring morphism, so it should be enough to look at the evaluation of one coefficient, or in fact of any element of the base ring.

So as I said in the other comment, I think it is okay to work around the fact that some of the base rings we may want to work with have an evaluation function that does not respect this constraint. However, I'm still not sure what you mean by “this value [...] only works for monomials”.

[fchapoton]

There may be a tension between landing in the "minimal parent" or the "best uniform parent". If you substitute into 0 in the ring QQ[x,y], you can always land in the ring QQ[x,y] itself, for any value of x and y.

As far as I know, we have no mechanism for substitution-coercion at the level of rings, do we ?

Do you want me to remove the comment, or write something else ?

[mezzarobba]

There may be a tension between landing in the "minimal parent" or the "best uniform parent". If you substitute into 0 in the ring QQ[x,y], you can always land in the ring QQ[x,y] itself, for any value of x and y.

I think the minimal uniform parent (i.e., the smallest parent that makes the operations you are implementing a well-defined map) is almost always the right choice — and is what we are typically using. In other words: usually, the parent of the result should only depend on the parents of the inputs. For p∈ℚ[x,y], I would say p(x=0,y=0) is in ℚ, but p(x=0) == p(x=0, y=y) is in ℚ[y] regardless of the value of p.

As far as I know, we have no mechanism for substitution-coercion at the level of rings, do we ?

I'm not sure what kind of mechanism you have in mind. Perhaps Morphism objects can play this role in a sense?

Do you want me to remove the comment, or write something else ?

I think the comment can be removed, but feel free to leave it or reword it if you feel there is something important to say.

[fchapoton]

ok, j'ai raccourci le commentaire

[github-actions]

Documentation preview for this PR (built with commit 956aaf0; changes) is ready! 🎉

[mezzarobba]

ok, thanks