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

Performance improvements #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Performance improvements #24

wants to merge 1 commit into from

Conversation

nsajko
Copy link

@nsajko nsajko commented Oct 12, 2022

Prevents run-time dispatch in the most basic PolynomialRoots.roots method.

findlast does not necessarily return an Integer, it may also return
Nothing, depending on the value of its arguments.

The latter case needs to be handled to prevent run-time dispatch for
the slicing operation.

Additionally, the truncated coefficients slice is now a view
instead of a copy. This should make things slightly more performant,
apart from making the code simpler.

JET warnings before this commit:

```
julia> @report_opt PolynomialRoots.roots(rand(5))
═════ 1 possible error found ═════
┌ @ /home/nsajko/.julia/packages/PolynomialRoots/1iZQh/src/PolynomialRoots.jl:610 PolynomialRoots.:(var"#roots#2")(PolynomialRoots.NaN, false, #self#, poly)
│┌ @ /home/nsajko/.julia/packages/PolynomialRoots/1iZQh/src/PolynomialRoots.jl:617 1 PolynomialRoots.:(:) %28
││ runtime dispatch detected: (1 PolynomialRoots.:(:) %28::Union{Nothing, Int64})::UnitRange{Int64}
│└─────────────────────────────────────────────────────────────────────────────────
```

After this commit:
```
julia> using JET, PolynomialRoots

julia> PolynomialRoots.roots(rand(5))
4-element Vector{ComplexF64}:
   -31.46526111710419 - 1.6981590262940853e-16im
 -0.13020601478750077 - 0.8162149121849699im
 -0.13020601478750077 + 0.81621491218497im
 -0.48806837350069937 + 2.350988701644575e-38im

julia> @report_opt PolynomialRoots.roots(rand(5))
No errors detected
```
src/PolynomialRoots.jl Outdated Show resolved Hide resolved
@giordano
Copy link
Owner

Do you have any benchmarks? Tests are failing, I'd be OK with requiring a newer version of Julia if that helps, but tests are failing also on nightly (haven't checked why)

@nsajko
Copy link
Author

nsajko commented Dec 13, 2022

The failing tests are for roots5, which shouldn't be changed at all by this (remaining) commit.

I can do some benchmarking if you want. JET still shows the same warning on Julia nightly, though, so this change still fixes the run time dispatch.

@nsajko
Copy link
Author

nsajko commented Dec 13, 2022

Regarding the tests that fail for Julia 1.0: the reason seems to be the begin indexing syntax.

The begin syntax also causes an error on Julia 1.4.2, but 1.5.4 seems to work fine.

@giordano
Copy link
Owner

but tests are failing also on nightly (haven't checked why)

Tests are failing on nightly independently of this PR. It looks like the result of

@test isapprox(zeros(length(poly) - 1),
evalpoly(@inferred(roots5(poly, ones(5))), poly), atol = 3e-14)
and
@test isapprox(zeros(length(poly) - 1),
evalpoly(@inferred(roots5(poly, ones(5))), poly), atol = 2e-14)
changed so that they go off the previously set absolute tolerance.

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

Successfully merging this pull request may close these issues.

2 participants