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

Cubic & Quintic Hermite Spline #280

Merged

Conversation

SouthEndMusic
Copy link
Member

Fixes #218.

Checklist

  • Appropriate tests were added
  • 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

Additional context

I am on a roll hacking around in this repository, so I decided to add the quintic Hermite interpolation as well (:

@SouthEndMusic SouthEndMusic changed the title Quintic hermite interpolation Quintic Hermite interpolation Jun 30, 2024
@ChrisRackauckas
Copy link
Member

I like it, but two things.

  1. It seems a little odd to have the quintic but not the cubic Hermite, that should probably be added.
  2. (u, t, du, ddu) seems like an odd argument order. (ddu, du, u, t) seems more natural

@sathvikbhagavan
Copy link
Member

  1. Yes, having cubic hermite makes sense.
  2. Yes, (ddu, du, u, t) would be better.

@SouthEndMusic SouthEndMusic changed the title Quintic Hermite interpolation Cubic & Quintic Hermite interpolation Jul 1, 2024
ChrisRackauckas
ChrisRackauckas previously approved these changes Jul 1, 2024
@ChrisRackauckas
Copy link
Member

BTW, I think you've made significant contributions, and we just happen to be writing an article on DataInterpolations right now. Would you like to a be co-author?

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! Had some minor comments. Also can you add a few tests for checking parameter caches are set properly or not similar to my comment in #274?

src/interpolation_caches.jl Outdated Show resolved Hide resolved
src/interpolation_caches.jl Outdated Show resolved Hide resolved
src/interpolation_caches.jl Outdated Show resolved Hide resolved
@SouthEndMusic
Copy link
Member Author

BTW, I think you've made significant contributions, and we just happen to be writing an article on DataInterpolations right now. Would you like to a be co-author?

That sounds interesting, what would that entail?

@sathvikbhagavan
Copy link
Member

We already have a JOSS paper review going on - openjournals/joss-reviews#6917

Can you make a PR adding your name and affiliation in https://github.com/SciML/DataInterpolations.jl/blob/master/joss/paper.md?

@SouthEndMusic SouthEndMusic changed the title Cubic & Quintic Hermite interpolation Cubic & Quintic Hermite Spline Jul 2, 2024
README.md Outdated Show resolved Hide resolved
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.

Apart from that typo.. LGTM!

Co-authored-by: Sathvik Bhagavan <[email protected]>
@ChrisRackauckas ChrisRackauckas merged commit 4934eb8 into SciML:master Jul 3, 2024
10 checks passed
@jer-j jer-j mentioned this pull request Jul 6, 2024
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.

Quintic Hermite Interpolation
3 participants