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

Added Linear() extrapolation method #93

Merged
merged 7 commits into from
Jul 8, 2024
Merged

Conversation

fintzij
Copy link
Contributor

@fintzij fintzij commented Jul 2, 2024

Added linear extrapolation method. The slope at the boundaries is calculated via finite differences.

(Hope the pull request is OK and this is helpful. Thanks for creating this package!)

@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 94.91%. Comparing base (a33f3c7) to head (ed6b81c).
Report is 9 commits behind head on master.

Files Patch % Lines
src/SplineExtrapolations/SplineExtrapolations.jl 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
- Coverage   95.61%   94.91%   -0.71%     
==========================================
  Files          35       35              
  Lines        2237     2224      -13     
==========================================
- Hits         2139     2111      -28     
- Misses         98      113      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jipolanco
Copy link
Owner

Thanks, this can be useful indeed!

I'd just be a bit careful about using finite differences with eps() increments for at least two reasons.

First, because calling eps() without argument is the same as calling eps(1.0), which only makes sense when the boundary location a or b is approximately 1 (and is a Float64). For example, note that 10.0 + eps() == 10.0. The solution is to use eps(a) instead (or nextfloat/prevfloat).

The second problem is that, if one takes two "consecutive" numbers x and y = x + eps(x) and evaluate a function f(x) at these two points, it is possible for the two results to be the same. For example:

julia> x = 2.0
2.0

julia> y = x + eps(x)      # or y = nextfloat(x)
2.0000000000000004

julia> sqrt(x) == sqrt(y)  # true!
true

A solution would be to use the actual derivatives of the spline at the boundary locations to perform the extrapolations. Right now BSplineKit.jl doesn't allow to get a derivative at a single point x only, but only the full derivative of the spline which must then be evaluated at x.

This will be clearer with an example:

using BSplineKit
xs = 0.2:0.2:10.2
ys = sqrt.(xs)
S = interpolate(xs, ys, BSplineOrder(4))
a, b = boundaries(basis(S))

To obtain the derivative of S at a, we could do the following:

S′ = Derivative() * S
S′(a)  # 1.0738781882215265

But this is actually overkill as it requires first computing the spline derivative S′, which can be relatively expensive if the spline has many points. A more efficient solution would be to obtain S′(a) using automatic differentiation:

using ForwardDiff
ForwardDiff.derivative(S, a)  # 1.0738781882215263

In my opinion we can follow that last strategy. This will require adding ForwardDiff.jl as a dependency, but that's fine for me.

@fintzij
Copy link
Contributor Author

fintzij commented Jul 4, 2024

Thanks, I agree that using ForwardDiff is a much better solution. I've pushed an update Linear() function but for some reason can't get the dependencies to work properly when I do a test run in the interpolations.jl file in the examples. Probably something to do with building the package locally. Does it work properly if you pull my changes in?

@jipolanco
Copy link
Owner

Not sure, but it may be related to the other changes made in Project.toml (besides adding ForwardDiff.jl).

@fintzij
Copy link
Contributor Author

fintzij commented Jul 4, 2024

Yup, that'll do it. Everything is working on my end now, I think it's ready for review.

hopefully this fixes the CI error
Project.toml Outdated Show resolved Hide resolved
docs/Project.toml Outdated Show resolved Hide resolved
notebooks/Project.toml Outdated Show resolved Hide resolved
src/BSplineKit.jl Outdated Show resolved Hide resolved
Project.toml Show resolved Hide resolved
removed unnecessary ForwardDiff inclusions, added Linear to extrapolation.md, removed CairoMakie from Project.toml
@fintzij fintzij requested a review from jipolanco July 6, 2024 00:57
@jipolanco
Copy link
Owner

Looks good to me, thanks!

@jipolanco jipolanco merged commit 32575ff into jipolanco:master Jul 8, 2024
4 checks passed
@jipolanco jipolanco mentioned this pull request Jul 8, 2024
@fintzij
Copy link
Contributor Author

fintzij commented Jul 8, 2024

Excellent, thank you too!

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