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

Use cubic instead of linear interpolation in predict #63

Merged
merged 1 commit into from
Apr 19, 2023

Conversation

andreasnoack
Copy link
Member

@andreasnoack andreasnoack commented Mar 24, 2023

The cubic interpolation is what is suggested in Cleveland and Grosee (1991) and also what R uses. It makes the prediction function once differentiable which is what I think most people would expect from LOESS.

Also, fix bug in use of partialsort!. Only the q'th element was ensured to be at the right localtion instead of all the first q elements.

Closes #61

@andreasnoack
Copy link
Member Author

andreasnoack commented Mar 24, 2023

The first test failure here is a bit surprising. For some reason the KD tree ends up not using the x values in

x = [13.0,14.0,14.35,15.0,16.0]
y = [0.369486, 0.355579, 0.3545, 0.356952, 0.36883]
model = loess(x,y)
@test Loess.predict(model,x) y
as vertices. It ends up with more vertices. Specifically

julia> model.verts
Dict{Vector{Float64}, Int64} with 6 entries:
  [14.0]  => 1
  [14.35] => 2
  [13.0]  => 3
  [16.0]  => 4
  [13.5]  => 5
  [15.5]  => 6

which means that there is no longer a perfect match for the x values. The match in the current master version seems to be somewhat coincidental. I'll try to figure out what is going on.

src/Loess.jl Outdated Show resolved Hide resolved
src/Loess.jl Outdated Show resolved Hide resolved
src/Loess.jl Outdated Show resolved Hide resolved
src/Loess.jl Show resolved Hide resolved
@andreasnoack
Copy link
Member Author

andreasnoack commented Mar 29, 2023

I think I've finally managed to figure out how to compute the splits, see #64. It was pretty tedious to reverse engineer. When that one is in, this PR can be updated and the broken test I added in #64 should then pass.

I've also realized that with this PR, we shouldn't store the polynomial coefficients from the local regressions. Instead, we should just store the predictions and the derivatives of the predictions at all the vertices. That is all we need to construct the cubic interpolations. It took me way too long to realize that these quantities are what R stores in the $kd$vval field.

@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2023

Codecov Report

Patch coverage: 96.66% and project coverage change: +1.08 🎉

Comparison is base (306d5de) 92.11% compared to head (1e4e6ac) 93.20%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   92.11%   93.20%   +1.08%     
==========================================
  Files           2        2              
  Lines         203      206       +3     
==========================================
+ Hits          187      192       +5     
+ Misses         16       14       -2     
Impacted Files Coverage Δ
src/Loess.jl 88.23% <96.66%> (+1.36%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/Loess.jl Outdated Show resolved Hide resolved
@andreasnoack andreasnoack force-pushed the an/cubic branch 3 times, most recently from 1b90daa to 1834385 Compare April 18, 2023 20:21
@andreasnoack
Copy link
Member Author

I think this one is ready now.

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

I guess it would be good to add some test for #61.

src/Loess.jl Outdated Show resolved Hide resolved
src/Loess.jl Outdated Show resolved Hide resolved
src/Loess.jl Outdated Show resolved Hide resolved
src/Loess.jl Outdated Show resolved Hide resolved
src/Loess.jl Show resolved Hide resolved
src/Loess.jl Outdated Show resolved Hide resolved
@andreasnoack
Copy link
Member Author

I guess it would be good to add some test for #61.

Please check the updated cars test. It should ensure that we interpolate exactly like the original implementation. Fr reference, I've recreated the plot from #61 with this branch
Skærmbillede 2023-04-19 kl  08 37 56

The cubic interpolation is what is suggested in Cleveland and
Grosee (1991) and also what R uses. It makes the prediction function
once differentiable which is what I think most people would
expect from LOESS.

Also, fix bug in use of partialsort!. Only the q'th element was
ensured to be at the right localtion instead of all the first q
elements.

Use evalpoly from Julia and bump minimum requirement to Julia 1.6
@andreasnoack
Copy link
Member Author

@devmotion I reapplied the changes you suggested

Copy link
Member

@devmotion devmotion 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 to me!

@andreasnoack andreasnoack merged commit 49fe426 into master Apr 19, 2023
@andreasnoack andreasnoack deleted the an/cubic branch April 19, 2023 11:19
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.

Predictions are not smooth
3 participants