SciML / DataInterpolations.jl

A library of data interpolation and smoothing functions
MIT License
229 stars 46 forks source link

Increasing performance by caching #271

Closed SouthEndMusic closed 4 months ago

SouthEndMusic commented 4 months ago

For my application I am looking at several features to improve the performance of interpolations:

I have implemented these features in a downstream package: https://github.com/SouthEndMusic/CachedInterpolations.jl (recently rebranded from SmoothInterpolation.jl, new name not registered yet)

Would you be interested in implementing these features in DataInterpolations.jl?

ChrisRackauckas commented 4 months ago

Yeah it would need benchmarks to go along with it but if it's hitting the performance then we'd definitely accept it.

SouthEndMusic commented 4 months ago

Regarding caching: do you want to support data modification after initialization of the interpolation object?

ChrisRackauckas commented 4 months ago

I'm not entirely sure what you're referring to.

SouthEndMusic commented 4 months ago

Currently there is a single source of truth for the data u,t and so something like this gives the expected result:

u = [1.0, 1.0, 1.0]
t = [1.0, 2.0, 3.0]

itp = LinearInterpolation(u, t)

u[2] = 2.0

itp(1.5) # 1.5

However, if you cache the slopes at the initialisation of itp this will no longer work.

ChrisRackauckas commented 4 months ago

I see. Yeah it's hard to disallow that, but the semantics we want are immutable semantics. I would think it's okay to note this limitation, though if we can find a clever way to error that would be good.

SouthEndMusic commented 4 months ago

The u, t fields can be wrapped by ReadOnlyArray from https://github.com/JuliaArrays/ReadOnlyArrays.jl so that they are at least not mutable when accessed as a field of the interpolation object.

ChrisRackauckas commented 4 months ago

That is a partial solution, but your example would still be an issue.

SouthEndMusic commented 4 months ago

The mutability of an existing object seems pretty immutable, better create a copy of u, t which are then wrapped in ReadOnlyArray if we're allocating for the cache in the constructor anyway.

ChrisRackauckas commented 4 months ago

That would be a nice way to do it. And we can have safetycopy = Val(true) for whether to enable/disable this extra copy.

SouthEndMusic commented 4 months ago

By the way, only this method of munge_data is not already copying u, t:

https://github.com/SouthEndMusic/DataInterpolations.jl/blob/088a9629a73e5ed52ed211422b227e159e8d11de/src/interpolation_utils.jl#L36-L38

SouthEndMusic commented 4 months ago

... but the semantics we want are immutable semantics.

With this in mind, should appending/pushing to u, t still be allowed, as happens in https://github.com/SciML/DataInterpolations.jl/blob/master/src/online.jl?

ChrisRackauckas commented 4 months ago

We should error saying this is only allowed if safetycopy is false?