Refactor subs() of multivariate polynomials for readability and efficiency

[kwankyu]

📚 Description

Fixes #34591

📝 Checklist

• I have made sure that the title is self-explanatory and the description concisely explains the PR.
• I have linked an issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation accordingly.

⌛ Dependencies

[codecov-commenter]

Codecov Report

Base: 88.57% // Head: 88.59% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (d49f43b) compared to base (52a81cb).
Patch has no changes to coverable lines.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #35210      +/-   ##
===========================================
+ Coverage    88.57%   88.59%   +0.02%     
===========================================
  Files         2140     2140              
  Lines       397273   397273              
===========================================
+ Hits        351891   351971      +80     
+ Misses       45382    45302      -80     

Impacted Files	Coverage Δ
src/sage/modular/local_comp/liftings.py	98.33% <0.00%> (-1.67%)	⬇️
src/sage/combinat/combination.py	93.50% <0.00%> (-1.50%)	⬇️
src/sage/homology/matrix_utils.py	87.15% <0.00%> (-0.92%)	⬇️
src/sage/modular/modform/numerical.py	94.19% <0.00%> (-0.65%)	⬇️
...sage/geometry/hyperbolic_space/hyperbolic_model.py	88.95% <0.00%> (-0.62%)	⬇️
src/sage/doctest/forker.py	80.24% <0.00%> (-0.54%)	⬇️
src/sage/graphs/generators/random.py	91.95% <0.00%> (-0.52%)	⬇️
src/sage/doctest/reporting.py	74.45% <0.00%> (-0.44%)	⬇️
src/sage/cpython/_py2_random.py	76.03% <0.00%> (-0.42%)	⬇️
src/sage/groups/cactus_group.py	98.89% <0.00%> (-0.37%)	⬇️
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mkoeppe]

This appears to be more general than the old code that tries to coerce every v into parent._base.
Is this just incidental, or is there a use case for this greater generality?

[kwankyu]

Perhaps there is no difference in this respect. Both first try to coerce every v into parent._base. If fails, the old code finds the common parent in return self(*g) (the last line) via the constructor __init__ while the new code does the same in the place. However, for some paths (substitutions with constant values), the new code does not make a duplicate call to singular_polynomial_call by avoiding going through the constructor. (if I remember correctly...)

[kwankyu]

Perhaps it is the substitution with constant values that results in zero that makes the duplicate call.

[github-actions]

Documentation preview for this PR is ready! 🎉
Built with commit: d49f43b

[mkoeppe]

LGTM

[vbraun]

Merge conflict

[kwankyu]

Fixed merge conflict with #33373

[mantepse]

@kwankyu, sorry to dig up this old PR. I want to fix #19130, that is, make parents properly depend on the arguments. Trying to figure out how things work I noticed something I do not understand:

https://github.com/kwankyu/sage/blob/8e0369193b7b83aaf0df8459ec75996ea18a3ba3/src/sage/rings/polynomial/multi_polynomial_libsingular.pyx#L3515-L3536

In the first line of the snippet, change_ring is introduced and set to False. I don't see that it would be modified before the last line of the snippet.

Do you remember the intention?

[kwankyu]

In the first line of the snippet, change_ring is introduced and set to False. I don't see that it would be modified before the last line of the snippet.

Do you remember the intention?

No. So I read the lines again.

Line 3536 (if not change_ring:) seems unnecessary because change_ring is False at that moment.

The main intention of introducing change_ring seems this:

        We test that we change the ring even if there is nothing to do::

            sage: P = QQ['x,y']
            sage: x = var('x')
            sage: parent(P.zero().subs(x=x))
            Symbolic Ring

[mantepse]

Well, there must have been some intention, because all of this code is new, and was done in an attempt to simplify the code. Could it be that change_ring should be modified before the if, but wasn't because of an oversight?

(I am trying to figure out what the parent of the result of subs should be in principle.)

[kwankyu]

Well, there must have been some intention, because all of this code is new, and was done in an attempt to simplify the code. Could it be that change_ring should be modified before the if, but wasn't because of an oversight?

The value of change_ring can change before lines 3590 and 3612. It seems that change_ring is intended for those lines. But the if condition of line 3536 is always satisfied, apparently.

(I am trying to figure out what the parent of the result of subs should be in principle.)

Yes. This is tricky. In most of the cases, the parent should not change. But there are exceptional cases where the parent should change. Like the zero polynomial above. Perhaps you should generalize this obvious case.