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

Introduce idx_prev to use as iguess #273

Merged
merged 6 commits into from
Jun 29, 2024

Conversation

SouthEndMusic
Copy link
Member

@SouthEndMusic SouthEndMusic commented Jun 25, 2024

Fixes half of #271

Checklist

  • Appropriate tests were added (n/a)
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

@SouthEndMusic SouthEndMusic marked this pull request as draft June 25, 2024 18:33
@SouthEndMusic SouthEndMusic changed the title Introduce idx_prev to use as iguess, consistently use idx Introduce idx_prev to use as iguess Jun 25, 2024
@SouthEndMusic
Copy link
Member Author

Quick benchmark:

using DataInterpolations
using BenchmarkTools

N = 1000
u = rand(N)
t = cumsum(rand(N))

itp = LinearInterpolation(u, t)

t_eval = range(first(t), last(t), length = 10000)

@btime itp.(t_eval)

# old: 276.000 μs (3 allocations: 78.25 KiB)
# new: 166.600 μs (3 allocations: 78.27 KiB)

So the better iguess gives a speedup because successive evaluations are close together. What kind of benchmarking would you like to see?

@SouthEndMusic SouthEndMusic marked this pull request as ready for review June 25, 2024 18:46
Copy link
Member

@sathvikbhagavan sathvikbhagavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Can you add some tests to check whether idx_prev is properly set?

test/interpolation_tests.jl Outdated Show resolved Hide resolved
for t in range(first(A.t), last(A.t); length = 2 * length(A.t) - 1)
A(t)
if hasproperty(A, :idx_prev)
@test abs(A.idx_prev[] -
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this test be written with equality?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the kwargs of the new function get_idx show, the index is obtained in several slightly different ways. Therefore it is not possible to test the index with equality in an interpolation method agnostic way :/

@@ -35,6 +36,11 @@ function test_derivatives(method, u, t; args = [], kwargs = [], name::String)
adiff2 = derivative(func, _t, 2)
@test isapprox(fdiff, adiff, atol = 1e-8)
@test isapprox(fdiff2, adiff2, atol = 1e-8)
# Cached index
if hasproperty(func, :idx_prev)
@test abs(func.idx_prev[] -
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this test be written with equality?

src/interpolation_utils.jl Outdated Show resolved Hide resolved
src/interpolation_utils.jl Show resolved Hide resolved
@sathvikbhagavan
Copy link
Member

@ChrisRackauckas, can you approve CI?

@ChrisRackauckas ChrisRackauckas merged commit 22dd3cf into SciML:master Jun 29, 2024
10 checks passed
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.

3 participants