Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ahbarnett opened this issue Jan 19, 2023 · 1 comment

Comments

@ahbarnett
Copy link

ahbarnett commented Jan 19, 2023

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

Polynomials.roots(p::Union{Polynomial{Float64},Polynomial{Complex{Float64}}}) = rootsFastPolynomialRoots(coeffs(p))

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

@ahbarnett ahbarnett changed the title awkward that you overwrite existing Polynomials.roots(), prevents comparing awkward that you overwrite existing Polynomials.roots(); prevents comparison Jan 19, 2023
@andreasnoack
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants