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.

Sign up for GitHub

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

Jump to bottom

ENH: add method arg to interpolate.Akima1DInterpolator #19696

Merged
merged 15 commits into from Dec 21, 2023
Merged

ENH: add method arg to interpolate.Akima1DInterpolator #19696

merged 15 commits into from Dec 21, 2023

Conversation

n-takumasa
Copy link
Contributor

@n-takumasa n-takumasa commented Dec 14, 2023 •
edited

Reference issue

No. But I found a discussion on the mailing list.

What does this implement/fix?

Add a functionally to use the modified Akima interpolation.

Additional information

https://www.mathworks.com/help/matlab/ref/makima.html

@n-takumasa n-takumasa marked this pull request as ready for review December 14, 2023 08:37
@lucascolley lucascolley added enhancement A new feature or improvement scipy.interpolate labels Dec 14, 2023
j-bowhay
j-bowhay reviewed Dec 14, 2023
View reviewed changes
scipy/interpolate/_cubic.py Outdated Show resolved Hide resolved
scipy/interpolate/tests/test_interpolate.py Outdated Show resolved Hide resolved
scipy/interpolate/tests/test_interpolate.py Show resolved Hide resolved
scipy/interpolate/_cubic.py Outdated
Comment on lines 387 to 389
modified : bool, optional
If ``True``, use the modified Akima interpolation[1991].
Defaults to ``False``, use the Akima interpolation[1970].
Copy link
Member

@j-bowhay j-bowhay Dec 14, 2023 •
edited

Choose a reason for hiding this comment

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

I think some additional documentation is needed, perhaps in the notes section, explain what the difference is and why you should consider using one or the other

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agreed, but I couldn't come up with a good sentence. 😢

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, we'd have explicit formulas for the slopes in the Notes section, similar to https://docs.scipy.org/doc/scipy/reference/generated/scipy.interpolate.PchipInterpolator.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote it based on references. Please let me know if there are any better expressions.

@n-takumasa @j-bowhay
Enforcing keyword arguments
e1c3ac7 
Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>
n-takumasa and others added 2 commits December 14, 2023 19:48
@n-takumasa @j-bowhay
Update scipy/interpolate/tests/test_interpolate.py
ace6d81 
Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>
@n-takumasa @j-bowhay
Update scipy/interpolate/tests/test_interpolate.py
5e8437c 
Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>
@n-takumasa @j-bowhay
Update scipy/interpolate/_cubic.py
d44aecd 
Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>
ev-br
ev-br requested changes Dec 14, 2023
View reviewed changes
Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

Looks reasonable, modulo an API tweak.

scipy/interpolate/_cubic.py Outdated Show resolved Hide resolved
scipy/interpolate/_cubic.py Outdated
Comment on lines 387 to 389
modified : bool, optional
If ``True``, use the modified Akima interpolation[1991].
Defaults to ``False``, use the Akima interpolation[1970].
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, we'd have explicit formulas for the slopes in the Notes section, similar to https://docs.scipy.org/doc/scipy/reference/generated/scipy.interpolate.PchipInterpolator.html

scipy/interpolate/_cubic.py Outdated Show resolved Hide resolved
@ev-br ev-br added the needs-work Items that are pending response from the author label Dec 14, 2023
@n-takumasa n-takumasa changed the title ENH: add modified arg to interpolate.Akima1DInterpolator ENH: add method arg to interpolate.Akima1DInterpolator Dec 15, 2023
j-bowhay
j-bowhay requested changes Dec 15, 2023
View reviewed changes
scipy/interpolate/_cubic.py Show resolved Hide resolved
scipy/interpolate/_cubic.py Show resolved Hide resolved
scipy/interpolate/_cubic.py Outdated Show resolved Hide resolved
scipy/interpolate/_cubic.py Outdated Show resolved Hide resolved
scipy/interpolate/_cubic.py Outdated Show resolved Hide resolved
@lucascolley lucascolley removed the needs-work Items that are pending response from the author label Dec 16, 2023
n-takumasa
n-takumasa commented Dec 18, 2023
View reviewed changes
scipy/interpolate/_cubic.py Outdated
Comment on lines 476 to 480
.. [2] A method of univariate interpolation that has the accuracy of
a third-degree polynomial. Hiroshi Akima, J. ACM, September 1991,
17(3), 341-366. :doi:`10.1145/114697.116810`
.. [3] Makima Piecewise Cubic Interpolation. Cleve Moler and Cosmin Ionita, 2019.
https://blogs.mathworks.com/cleve/2019/04/29/makima-piecewise-cubic-interpolation/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
.. [2] A method of univariate interpolation that has the accuracy of
a third-degree polynomial. Hiroshi Akima, J. ACM, September 1991,
17(3), 341-366. :doi:`10.1145/114697.116810`
.. [3] Makima Piecewise Cubic Interpolation. Cleve Moler and Cosmin Ionita, 2019.
https://blogs.mathworks.com/cleve/2019/04/29/makima-piecewise-cubic-interpolation/
.. [2] Makima Piecewise Cubic Interpolation. Cleve Moler and Cosmin Ionita, 2019.
https://blogs.mathworks.com/cleve/2019/04/29/makima-piecewise-cubic-interpolation/

I misunderstood.

% We modify Akima's formula to eliminate overshoot and undershoot
% when the data is constant for more than two consecutive nodes.

https://blogs.mathworks.com/cleve/2019/04/29/makima-piecewise-cubic-interpolation/?from=jp#:~:text=interpolation.%0A%25%0A%25-,We%20modify,-Akima%27s%20formula%20to

j-bowhay
j-bowhay approved these changes Dec 20, 2023
View reviewed changes
Copy link
Member

@j-bowhay j-bowhay left a comment

Choose a reason for hiding this comment

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

LGTM thanks @n-takumasa, will wait to see if @ev-br is happy before merging

ev-br
ev-br approved these changes Dec 21, 2023
View reviewed changes
Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

LGTM

@lucascolley lucascolley added this to the 1.13.0 milestone Dec 21, 2023
@j-bowhay j-bowhay merged commit 1c38ce5 into scipy:main Dec 21, 2023
26 of 27 checks passed
@n-takumasa n-takumasa deleted the modified_akima branch December 23, 2023 02:56
@psmd-iberutaru psmd-iberutaru mentioned this pull request Mar 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucascolley lucascolley lucascolley left review comments

@ev-br ev-br ev-br approved these changes

@j-bowhay j-bowhay j-bowhay approved these changes

Assignees
No one assigned
Labels
enhancement A new feature or improvement scipy.interpolate
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@n-takumasa @ev-br @lucascolley @j-bowhay