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 NdBSpline: n-dim tensor product b-spline object #18492

Merged
merged 21 commits into from Jun 8, 2023

Conversation

ev-br
Copy link
Member

@ev-br ev-br commented May 20, 2023

Reference issue

cross-ref #18010, #7903

What does this implement/fix?

Add a new class NdBSpline, to represent an N-dimensional tensor product b-spline. This class only knows how to evaluate a tensor product given coefficients and knot vectors. This way it generalizes BSpline for 1D data to N-D, and parallels NdPPoly (which represents N-D tensor product polynomials in the power basis).

This class is intended to serve as a basis for a further class/routine to construct an N-D interpolator given multidim data, possibly internally in RegularGridInterpolator. The latter currently uses a recursive application of make_interp_spline, which is very slow.
However constructing an interpolator from data is separate and is not a part of this PR.

One other thing we get for free from this class is evaluation of gradients, cf #7903 --- which gets messy for the recursive approach.

Additional information

This class uses the localized nature of b-splines, and goes to some lengths to accomodate a general ND case.
A straightforward implementation is available in https://github.com/ev-br/ndbsplne_playground/blob/main/NdBSpline_playground.ipynb , see NdBSpline and NdBSpline0 classes there.

This might be of interest to @Kai-Striega @j-bowhay and @AtsushiSakai who worked on similar things recently.
I'll also take liberty to ping @charris who discussed these things before.

Copy link
Member

@Kai-Striega Kai-Striega left a comment

Choose a reason for hiding this comment

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

Looks really cool. Well done!

I don't have the time for a full review tonight, but I'll try to get one done this weekend.

One thing: I don't have much of a background in this; do you have any resources to get an understanding of the maths going on here?

scipy/interpolate/_bspl.pyx Outdated Show resolved Hide resolved
scipy/interpolate/_ndbspline.py Outdated Show resolved Hide resolved
scipy/interpolate/_ndbspline.py Outdated Show resolved Hide resolved
scipy/interpolate/tests/test_bsplines.py Show resolved Hide resolved
scipy/interpolate/tests/test_bsplines.py Show resolved Hide resolved
@ev-br
Copy link
Member Author

ev-br commented May 20, 2023

One thing: I don't have much of a background in this; do you have any resources to get an understanding of the maths going on here?

Do you mean the general math behind tensor products or the tricks this PR does with jumping through the hoops of a dealing with arrays of runtime-defined dimensionality? For the general background, Chap 7 of Ref [1] of BSpline docstring is nice (https://www.uio.no/studier/emner/matnat/ifi/nedlagte-emner/INF-MAT5340/v05/undervisningsmateriale/hele.pdf) these lecture notes did help me a lot when I first looked at this; For what' s done here I just added some blurb to the top of https://github.com/ev-br/ndbsplne_playground/blob/main/NdBSpline_playground.ipynb, let me know if it's what you're looking for and what's missing.

@ev-br
Copy link
Member Author

ev-br commented May 20, 2023

No idea what mypy wants from me.
4549d9e does not seem to fix it.

@Kai-Striega
Copy link
Member

Do you mean the general math behind tensor products or the tricks this PR does with jumping through the hoops of a dealing with arrays of runtime-defined dimensionality?

I meant the general maths. I haven't done any real work with splines before and would like to learn more. I'll take a look at the link and get back to you. It does seem like a good introduction.

@tupui tupui added the Cython Issues with the internal Cython code base label May 21, 2023
@tupui tupui added this to the 1.12.0 milestone May 22, 2023
Copy link
Member

@Kai-Striega Kai-Striega left a comment

Choose a reason for hiding this comment

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

I still haven't had the time to review the maths literature. But the code looks good to me. I would prefer it if someone familiar with the maths could give this a review before we merge it.

scipy/interpolate/_ndbspline.py Show resolved Hide resolved
scipy/interpolate/_ndbspline.py Show resolved Hide resolved
scipy/interpolate/_ndbspline.py Outdated Show resolved Hide resolved
@Kai-Striega
Copy link
Member

@ev-br any progress on getting mypy happy? I think this is a mypy issue, not an issue with your code. If you need to, you can append # type: ignore to line 6 to make the tests pass. We should probably make sure it's documented somewhere.

scipy/interpolate/_ndbspline.py Outdated Show resolved Hide resolved
scipy/interpolate/_ndbspline.py Outdated Show resolved Hide resolved
ev-br and others added 2 commits May 29, 2023 14:11
Co-authored-by: Kai <kaistriega+github@gmail.com>
@ev-br
Copy link
Member Author

ev-br commented May 29, 2023

any progress on getting mypy happy? I think this is a mypy issue, not an issue with your code. If you need to, you can append # type: ignore to line 6 to make the tests pass. We should probably make sure it's documented somewhere.

I rebased on main and the lint CI seems to pass now. Documenting this would be great actually.

EDIT: Spoke too soon. Mypy still shows the same error.

EDIT2: adding # type: ignore to import _bspl seems to do the trick indeed. Thank you @Kai-Striega !

@Kai-Striega
Copy link
Member

@ev-br is there anything else that you are wanting to add to this PR, or are we only waiting on a reviewer for the maths?

@AtsushiSakai
Copy link
Member

Sorry for my late review. My comments are only related to docs, so can be ignored to merge.

Co-authored-by: Atsushi Sakai <asakai.amsl+github@gmail.com>
@ev-br
Copy link
Member Author

ev-br commented Jun 5, 2023

Thank you @AtsushiSakai , the last commit addresses your suggestions.

@Kai-Striega :

is there anything else that you are wanting to add to this PR, or are we only waiting on a reviewer for the maths?

I think this PR is done from my side. Constructing an interpolator from data + retrofitting it into RegularGridInterpolator can I think be done separately. If there is a preference the contrary, this PR can of course wait for that other part to be implemented.

@Kai-Striega
Copy link
Member

I'm happy with the PR as is. Let's leave it for a couple of days and if there's no more feedback I'll merge it. Thanks for your work @ev-br!

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.

Some minor points, the rest LGTM

scipy/interpolate/_ndbspline.py Outdated Show resolved Hide resolved
scipy/interpolate/_ndbspline.py Outdated Show resolved Hide resolved
scipy/interpolate/_ndbspline.py Show resolved Hide resolved
scipy/interpolate/_ndbspline.py Outdated Show resolved Hide resolved
scipy/interpolate/_ndbspline.py Outdated Show resolved Hide resolved
scipy/interpolate/_ndbspline.py Show resolved Hide resolved
scipy/interpolate/_ndbspline.py Outdated Show resolved Hide resolved
Copy link
Member Author

@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.

Thanks Jake, merged in your suggestions.

Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>
@Kai-Striega Kai-Striega merged commit 852e228 into scipy:main Jun 8, 2023
24 checks passed
@Kai-Striega
Copy link
Member

Merged :) Thanks for your work @ev-br, @AtsushiSakai and @j-bowhay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cython Issues with the internal Cython code base enhancement A new feature or improvement scipy.interpolate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants