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

Add optional GCV calculation #67

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ZeroCool2u
Copy link

This commit adds support for optionally calculating the Generalized Cross Validation criterion inline and making it available as a property of the SplinePPForm object.

@espdev I tried to minimize the API surface changes and made it fully optional, so it should have little to no performance impact on those that aren't interested in using the GCV.

Whenever you're good with this PR, I'll submit a separate one to try and include a simple example of how to actually use this value to minimize the GCV and generate an 'optimal' spline.

It's important to note that the GCV is not perfect and in some circumstances may not produce what one would visually consider to be the optimal smoothing parameter, but it does a pretty good job most of the time.

Also, it's quite expensive to calculate manually/outside of csaps as I've been doing, because it means you need to do roughly double the work unless you customize and then rebuild csaps entirely with the changes I've proposed here.

This commit adds support for optionally calculating the Generalized Cross Validation criterion inline and making it available as a property of the SplinePPForm object.
@espdev
Copy link
Owner

espdev commented Jun 12, 2023

Hello @ZeroCool2u,

Thank you for the contribution.
Could you add some tests coverage the new functionality?

csaps/_sspumv.py Outdated Show resolved Hide resolved
@ZeroCool2u
Copy link
Author

Happy to add tests and make the changes you asked for.

However, I do need to refactor this slightly. I forgot that the final line of computation requires the predicted values from the spline after it's constructed, so it won't work as is.

What I was thinking about doing instead was adding a new method to the spline object that takes the predicted values as an input and then returns or sets the gcv.

This would require that we store all of the intermediate values we need as part of the spline object or just the degrees of freedom. This will probably end up marginally increasing persistent memory usage though, so I want to make sure you're okay with that or if you have any other suggestions as to how you'd like me to go about it.

@espdev
Copy link
Owner

espdev commented Jun 13, 2023

You can even create a new inherited Spline class with an implementation of gcv, or you can embed this functionality in the existing class. The main thing is that this does not break backward compatibility and followed the rule: "don't pay for what you don't use", everything else is up to you. :)

@ZeroCool2u
Copy link
Author

Let me know if these changes look good to you. If so, I'll start working on setting up some tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants