Refactor polysubst.jl

[lgoettgens]

Contains #1888 to avoid conflicts.

This goes in hand with Nemocas/Nemo.jl#1933, but should hopefully be non-breaking on its own. Let's wait for CI to verify that.

[codecov]

Codecov Report

Attention: Patch coverage is 60.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.13%. Comparing base (f90e000) to head (ade496d).

Files with missing lines	Patch %	Lines
src/Poly.jl	66.66%	4 Missing ⚠️
src/NCPoly.jl	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1889      +/-   ##
==========================================
- Coverage   88.14%   88.13%   -0.01%     
==========================================
  Files         120      119       -1     
  Lines       30288    30290       +2     
==========================================
- Hits        26696    26695       -1     
- Misses       3592     3595       +3     

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

[fingolfin]

Fine by me but unfortunately it breaks HeckeCI tests:

Error During Test at /home/runner/.julia/packages/Hecke/p6Ffl/test/NumField/Hilbert.jl:1
  Got exception outside of a @test
  MethodError: (::ZZPolyRingElem)(::zzModPolyRingElem) is ambiguous. Candidates:
    (f::PolyRingElem)(a::PolyRingElem) in AbstractAlgebra at /home/runner/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/Poly.jl:3363
    (f::ZZPolyRingElem)(a::RingElem) in Nemo at /home/runner/.julia/packages/Nemo/0TzOv/src/polysubst.jl:27
    (f::PolyRingElem)(a::RingElem) in AbstractAlgebra at /home/runner/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/Poly.jl:3371
    (f::ZZPolyRingElem)(a) in Nemo at /home/runner/.julia/packages/Nemo/0TzOv/src/polysubst.jl:16
  Possible fix, define
    (::ZZPolyRingElem)(::PolyRingElem)

Also OscarCI with a similar one

[thofma]

Good look with the ambiguities.

It is a bit hard to check that this does not introduce regressions. Accidentally this could change things that used to compute f(g) as compose(f, g) to suddenly use evaluate(f, g) instead.

[lgoettgens]

The ambiguities only occur if we only have this patch in but not the one for Nemo. So let me mark this as breaking, and we can apply it with the next breaking release.

Regarding @thofma s comment: I don't see how this could be possible. The only difference is that if one calls f(g) for different types of Polys, then this gets delegated to subst via a different call function than before.

Edit: maybe there are some changes regarding stacks of poly rings. I'll play around with it

[thofma]

Regarding @thofma s comment: I don't see how this could be possible. The only difference is that if one calls f(g) for different types of Polys, then this gets delegated to subst via a different call function than before.

All I was trying to say is that we (the reviewers) have to check a bit more carefully, since for many argument combinations, both compose(f, g) and evaluate(f, g) give the right result, and so regressions are not detected by the tests.

[fingolfin]

since for many argument combinations, both compose(f, g) and evaluate(f, g) give the right result, and so regressions are not detected by the tests.

OK but if there are combinations were only one (or neither 😨 ) gives right results, then surely the very first thing we should do is to add those to the tests ... :-)