search

andreasnoack / FastPolynomialRoots.jl

Fast and backward stable computation of roots of polynomials in Julia
MIT License
15 stars 4 forks source link

awkward that you overwrite existing Polynomials.roots(); prevents comparison #23

Open ahbarnett opened 1 year ago

ahbarnett commented 1 year ago

Thanks for this exciting package, which I am benchmarking against AMRVW.jl, Polynomials.jl, PolynomialRoots.jl, and my own companion matrix eigval implementation.

However, because your package chooses to redefine the roots() method from the Polynomials module, one cannot actually run benchmarks, unless they are done as in your README.md where you can only test Polynomials.roots() before you load in FPR. I want to be able to load both modules, then call one or the other roots() function but cannot because either using or import on your pkg appears to redefine Polynomials.roots(). I think this is not good behavior; FPR should keep its own namespace and not change another pkg.

The offending line is https://github.com/andreasnoack/FastPolynomialRoots.jl/blob/17f0ab44e0213e2dd57161bbe23c52753403c17a/src/FastPolynomialRoots.jl#L5

Is there something I'm missing here (relatively new to Julia)? Thanks, Alex

andreasnoack commented 1 year ago

The original idea here was to speed up the existing implementation. However, this kind of piracy can indeed be confusing and cause issues so I can probably be convinced to change the current behavior and instead introduce a new function.