Breaking change introduced by #487

[baggepinnen]

This PR introduced a breaking change

• Eltype fix; use T[] for 0-polynomial; speed up LaurentPolynomial operations #487

more specifically,

Uses T[] for the coefficients of a 0 polynomial, not T[0], which requires zero(T). This led to some fixes on evaluation and changed some tests

This broke downstream use in ControlSystems.jl. The change does not really make total sense to me, these two polynomials should have the same number of coefficients, they only differ in their numerical values

julia> Polynomial(1).coeffs
1-element Vector{Int64}:
 1

julia> Polynomial(0).coeffs
Int64[]

0 is a coefficient after all and 0 != nothing. In particular since a 0 is returned for the 0:th degree coefficient in this case, introducing an inconsistency

julia> Polynomial([0,1])
Polynomial(x)

julia> Polynomial([0,1]).coeffs
2-element Vector{Int64}:
 0
 1

[jverzani]

Sorry, that was supposed to be an implementation detail and not a breaking change. It was introduced to make consistent with how we chop trailing 0s. But has this unintended consequence. Let me look to correcting it and add ControlSystems to the packages tested so as not to introduce any new breakage moving forward.

[jverzani]

So, I realize you made a change to ControlSystems to work around this issue. I can revert the old behavior in Polynomials, but unless you need it, I'd just prefer to add ControlSystems to the downstream packages that get tested so no future unintended consequences happen. Is that enough for your package?

[baggepinnen]

The workaround in CS is ok for our purposes, and I much appreciate the downstream testing :)

[ranocha]

I think this affects us in BSeries.jl, too. With Polynomials.jl v3.2.8, I get

(BSeries) pkg> add Polynomials@v3.2.8
    Updating registry at `~/.julia/registries/General.toml`
   Resolving package versions...
    Updating `~/.julia/dev/BSeries/Project.toml`
⌃ [f27b6e38] ↓ Polynomials v3.2.9 ⇒ v3.2.8
    Updating `~/.julia/dev/BSeries/Manifest.toml`
⌃ [f27b6e38] ↓ Polynomials v3.2.9 ⇒ v3.2.8
        Info Packages marked with ⌃ have new versions available and may be upgradable.
Precompiling environment...
  1 dependency successfully precompiled in 3 seconds. 12 already precompiled.

julia> using Polynomials

julia> v = Polynomial{Rational{Int}, :x}([0//1])
Polynomial(0//1)

julia> integrate(v)
Polynomial(0//1)

With Polynomials.jl v3.2.9, I get

(BSeries) pkg> add Polynomials@v3.2.9
    Updating registry at `~/.julia/registries/General.toml`
   Resolving package versions...
    Updating `~/.julia/dev/BSeries/Project.toml`
  [f27b6e38] ↑ Polynomials v3.2.8 ⇒ v3.2.9
    Updating `~/.julia/dev/BSeries/Manifest.toml`
  [f27b6e38] ↑ Polynomials v3.2.8 ⇒ v3.2.9
Precompiling environment...
  1 dependency successfully precompiled in 3 seconds. 12 already precompiled.

julia> using Polynomials

julia> v = Polynomial{Rational{Int}, :x}([0//1])
Polynomial(0//1)

julia> integrate(v)
Polynomial(0.0)

Note that the return type of integrate switched to a Polynomial with Float64 coefficients instead of being type-stable and keeping the Rational{Int} element type. This breaks our tests in BSeries.jl.

[jverzani]

Thanks. That is a slightly different issue and one that definitely needs to get fixed. I'll try and get a PR to do so soon.

[ranocha]

Great, thanks!

[jverzani]

The issue in BSeries.jl should be closed with #496

[ranocha]

Thanks a lot for the quick fix! I will restart CI with the new version of Polynomials.jl.